fab94534b6 doc: Document that wallet salvage is experimental (MarcoFalke)
Pull request description:
See #20151
ACKs for top commit:
practicalswift:
ACK fab94534b6: user safety first
hebasto:
ACK fab94534b6, maybe capitalize into "WARNING"?
meshcollider:
Trivial ACK fab94534b6
Tree-SHA512: 94912c491facc485293e4333066057933d706d84c7172f615296e7ba998c583c8bd07e751e6f00cd6576e7791007ace321f959181f7bf6a4e15e10d7ec8a1b7e
ad5cef5dfd doc: Update data directory path comments (Hennadii Stepanov)
b19e88230f util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
Pull request description:
Wallet names in `listwalletdir` RPC are correct now, even if the `-datadir` path has any number of trailing `/`.
This PR is an alternative to #19933.
Fixes #19928.
ACKs for top commit:
MarcoFalke:
review ACK ad5cef5dfd🔙
promag:
Code review ACK ad5cef5dfd.
meshcollider:
Code review + test run ACK ad5cef5dfd
Tree-SHA512: bccabbd6c18243d48d15b2b27201cc0f5984623dcbc635c8740cf74523f359844c36eadd40391142874fcf452a43880bb6afbf89815ae736e499f9a98143a661
6c0259fc2f Squashed 'src/secp256k1/' changes from c6b6b8f1bb..3967d96bf1 (Pieter Wuille)
Pull request description:
Nothing important changed, but this silences this (erroneous) warning in certain GCC 9 versions:
```
In file included from src/secp256k1.c:16:
src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
src/ecmult_impl.h:496:48: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
496 | secp256k1_gej tmp = a[state->ps[np].input_pos];
| ~~~~~~~~~~~~~^~~~~~~~~~
src/ecmult_impl.h:565:42: note: while referencing ‘ps’
565 | struct secp256k1_strauss_point_state ps[1];
| ^~
src/ecmult_impl.h:502:139: warning: array subscript [1, 268435456] is outside array bounds of ‘struct secp256k1_strauss_point_state[1]’ [-Warray-bounds]
502 | secp256k1_fe_mul(state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), state->zr + np * ECMULT_TABLE_SIZE(WINDOW_A), &(a[state->ps[np].input_pos].z));
| ~~~~~~~~~~~~~^~~~~~~~~~
src/ecmult_impl.h:565:42: note: while referencing ‘ps’
565 | struct secp256k1_strauss_point_state ps[1];
| ^~
```
(see https://github.com/bitcoin-core/secp256k1/issues/834)
ACKs for top commit:
fanquake:
ACK 5803f5f5f6 - performed the update myself and got the same change: [check_20257_subtree](https://github.com/fanquake/bitcoin/tree/check_20257_subtree).
hebasto:
ACK 5803f5f5f6, tested on Linux Mint 20 (x86_64) with `gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0` -- no warnings are emitted.
Tree-SHA512: 386281d23aee93a3b1d1a09fec8319c3a477e46967430c935677eed54abddc62d5a7710f9eeab1ec476ace05adcb194b5b377712e44a6bb95a74ffa35faf77f3
398045ba8b cli -netinfo: print oversized/extreme ping times as "-" (Jon Atack)
773f4c99c0 cli -netinfo: handle longer tor v3 local addresses (Jon Atack)
33e987452f cli -netinfo: make age column variable-width (Jon Atack)
f8a1c4d946 cli -netinfo: various quick updates and fixes (Jon Atack)
Pull request description:
Quick fixups and updates for v0.21.0:
- [x] handle larger BIP155 `addrv2` addresses
- [x] add Signet chain
- [x] add an additional space between the `net` and `mping` columns; add missing `tinyformat` and `algorithm` headers
- [x] s/uptime/age/ per 0xB10C suggestion, and make the column auto-adjusting variable width
- [x] display `-` for oversized mping/ping times like `1.17348e+06`, as reported by practicalswift
Edit: removed the release note commit, as this PR was not merged before the notes were moved to the wiki. It's here:
```
- A new `bitcoin-cli -netinfo` command returns a network peer connections
dashboard that displays data from the `getpeerinfo` and `getnetworkinfo` RPCs
in a human-readable format. An optional integer argument from `0` to `4` may
be passed to see various levels of detail. (#19643)
```
ACKs for top commit:
michaelfolkson:
ACK 398045ba8b
Emzy:
Tested ACK 398045ba8b
Tree-SHA512: 0625ee840141bafbfcaf8f1fce53f8f850ae91721b2bdad4279372da87c18a1fe3a214d90bfdbbabdf6da38d58290d7dd0f1109b4e2ca5d20cacf417d6ced0f9
bbb42a6896 RPC: createwallet: Nicer error message if descriptor wallet requested and sqlite support not compiled in (Luke Dashjr)
6608fec332 GUI: Create Wallet: Nicely disable descriptor wallet checkbox if sqlite support not compiled in (Luke Dashjr)
7b54d768e1 Make sqlite support optional (compile-time) (Luke Dashjr)
Pull request description:
As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21.
Potential follow-up PRs after this:
* Make BDB support optional
* Nicer error messages when user tries to load an unsupported wallet
* Don't compile descriptor wallet code if sqlite disabled
ACKs for top commit:
jonasschnelli:
Tested ACK bbb42a6896
achow101:
ACK bbb42a6896
Sjors:
re-utACK bbb42a6896
hebasto:
ACK bbb42a6896, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
Tree-SHA512: 500209dd1971310fab8ae51543343ce0ba91f088ccccff6109b4cc27547cd5532289dca6cb7dac2a7d7c59cdf3c8f5aacc31e9b0f912e38cea52ec26b97100bd
d419fdedbe [net processing] Don't add AlreadyHave txs to recentRejects (Troy Giorshev)
Pull request description:
If we already have a transaction, don't add it to recentRejects
Now, we only add a transaction to our recentRejects filter if we didn't already have it, meaning that it is added at most once, as intended.
ACKs for top commit:
jnewbery:
Code review ACK d419fdedbe
laanwj:
Code review ACK d419fdedbe
Tree-SHA512: cff5c1ba36c4700e2d6ab3eec4a3e51e1bef28fb3cc1bc850c84e06d6e5a9f6c32825207c253cc9cdf596b2eaadb6b5be68b3f8ca752b4ef6c31cf85138e3c99
3967d96bf1 Merge #838: Make autotools check for all the used openssl functions
3734b68200 Configure echo if openssl tests are enabled
6f54e69f03 Merge #841: Avoids a potentially shortening size_t to int cast in strauss_wnaf_
8893f42438 Avoids a potentially shortening size_t to int cast in strauss_wnaf_
e6692778d3 Modify bitcoin_secp.m4's openssl check to call all the functions that we use in the tests/benchmarks. That way linking will fail if those symbols are missing
ac05f61fcf Merge #809: Stop treating ECDH as experimental
e6e3d5da2f travis: add schnorrsig to valgrind and big endian platform test
353dff156f Stop treating ECDH as experimental
git-subtree-dir: src/secp256k1
git-subtree-split: 3967d96bf184519eb98b766af665b4d4b072563e
- add new signet chain
- update change "uptime" column name to "age" per suggestion by 0xB10C (Timo)
- add an additional digit to mping field width
- change m_networks_size from size_t to uint8_t, as size_t was a holdover
from m_networks_size being defined as size_t m_networks.size() in a draft
- order Peer struct members by decreasing memory size
20c9e03554 gui: Call setWalletActionsEnabled(true) only for the first wallet (Hennadii Stepanov)
Pull request description:
On master (a78742830a) there is a bug:
- open an encrypted wallet; please note that the "Encrypt Wallet..." menu item is disabled that is expected:
- then open any other wallet; note that the "Encrypt Wallet..." menu item gets enabled that is wrong:
This PR fixes this bug.
ACKs for top commit:
jonasschnelli:
Tested ACK 20c9e03554 - I could reproduce the issue on master and have verify that this PR fixes it.
achow101:
ACK 20c9e03554
Tree-SHA512: 2c9ab94bde8c4f413b0a95c05bf3a1a29f5910e0f99d6639a11dd77758c78af25b060b3fecd78117066ef15b113feb79870bc1347cc04289da915c00623e5787
56a461f727 wallet: fix buffer over-read in SQLite file magic check (Sebastian Falbesoner)
Pull request description:
Looking at our new SQLite database code, I noticed that there is a potential problem in the method `IsSQLiteFile()`: If there is no terminating zero within the first 16 bytes of the file, the `magic` buffer would be over-read in the `std::string` constructor for `magic_str`. Fixed by using the "from buffer" variant of the string ctor (that also takes a size) rather than the "from c-string" variant (see http://www.cplusplus.com/reference/string/string/string/).
The behaviour can be reproduced by the following steps:
* Creating a file of at least 512 bytes in size (to pass the minimum size check) that doesn't contain zero bytes in the magic area, e.g. simply:
`$ python3 -c "print('A'*512)" > /tmp/corrupt_wallet`
* Showing content and size of the `magic_str` string in case the magic check fails
* Create a simple unit test that simply calls `IsSQLiteFile` with the corrupt wallet file
* Run the unit test and see the random gibberish output of `magic_str` after 16 `A`s :-)
Or, TLDR variant, just get the branch https://github.com/theStack/bitcoin/tree/reproduce_sqlite_magic_overread, compile unit Tests and run the script `./reproduce_sqlite_magic_overread.sh`.
Note that this is the minimal diff, probably it would be better to avoid `std::string` at all in this case and just use `memcmp`, strings that include null bytes are pretty confusing.
ACKs for top commit:
promag:
Code review ACK 56a461f727.
practicalswift:
ACK 56a461f727: patch looks correct
achow101:
ACK 56a461f727
Tree-SHA512: a7aadd4d38eb92337e6281df2980f4bde744dbb6cf112b9cd0f2cab8772730e302db9123a8fe7ca4e7e844c47e68957487adb2bed4518c40b4bed6a69d7922b4
If there is no terminating zero within the 16 magic bytes, the buffer would be
over-read in the std::string constructor. Fixed by using the "from buffer"
variant of the ctor (that also takes a size) rather than the "from c-string"
variant.
fa4074b395 Show name, format and if uses descriptors in bitcoin-wallet tool (Jonas Schnelli)
Pull request description:
ACKs for top commit:
MarcoFalke:
ACK fa4074b395
jonatack:
re-ACK fa4074b395
Tree-SHA512: cf6ee96ff21532fc4b0ba7a0fdfdc1fa485c9b1495447350fe65cd0bd919e0e0280613933265cdee069b8c29ccf015ac374535a70cac3d4fb89f4d08b3a03519
2d5793c016 Bugfix: chainparams: Add missing (disabled) Taproot deployment for Signet (Luke Dashjr)
Pull request description:
Is there a way we can trigger compiler warnings if a deployment is undefined?
ACKs for top commit:
decryp2kanon:
utACK 2d5793c016
MarcoFalke:
review ACK 2d5793c016
Tree-SHA512: 135cefae0f8dc552b0f682c2b87cabca7a4716290a36410a55968850e803a5049234e3cc597c8ef8d7917ae5d5ea3fb851e160df171b6793114c6bc01c5ea3e7
624bab00dd test: add coverage for getwalletinfo format field (Jon Atack)
5e737a0092 rpc, wallet: Expose database format in getwalletinfo (João Barbosa)
Pull request description:
Support for sqlite based wallets was added in #19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`.
ACKs for top commit:
jonatack:
Tested ACK 624bab00dd
laanwj:
Code review ACK 624bab00dd.
MarcoFalke:
doesn't hurt ACK 624bab00dd
hebasto:
ACK 624bab00dd, tested on Linux Mint 20 (x86_64).
meshcollider:
utACK 624bab00dd
Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
c8abbc9d1f p2p: declare Announcement::m_state as uint8_t, add getter/setter (Jon Atack)
Pull request description:
Change `Announcement::m_state` in `tx_request.cpp` from type `State` to `uint8_t` and add a getter and setter for the conversion to/from `State`. This should silence these travis ci gcc compiler warnings:
```
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
too small to hold all values of ‘enum class {anonymous}::State’
State m_state : 3;
^
```
The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414.
ACKs for top commit:
sipa:
utACK c8abbc9d1f
ajtowns:
ACK c8abbc9d1f -- quick code review
hebasto:
ACK c8abbc9d1f, tested on Bionic (x86_64, gcc 7.5.0):
Tree-SHA512: 026721dd7a78983a72da77638d3327d2b252bef804e489278a852f000046c028d6557bbd6c2b4cea391d4e01f9264a1be842d502047cb90b2997cc37bee59e61
fa48405ef8 Warn on unknown rw_settings (MarcoFalke)
Pull request description:
Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.
Something similar is already done for the command line and config file. See:
* test: Add test for unknown args #16234 (commit fa7dd88b71)
ACKs for top commit:
ryanofsky:
Code review ACK fa48405ef8. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
Tree-SHA512: cec7d88adf84fa0a842f56b26245157736eb50df433db951e622ea07fd145b899822b24cdab1d8b36c066415ce4f0ef09b493fa8a8d691532822a59c573aafa7
76bbcc414f test: Fix -Wunused-function warning if configured --without-libs (Hennadii Stepanov)
Pull request description:
On master (80c8a02f1b) compiling with gcc:
```
$ ./configure --without-libs
$ make clean && make
...
test/script_tests.cpp:1369:23: warning: ‘CScriptWitness script_tests::ScriptWitnessFromJSON(const UniValue&)’ defined but not used [-Wunused-function]
1369 | static CScriptWitness ScriptWitnessFromJSON(const UniValue& univalue)
| ^~~~~~~~~~~~~~~~~~~~~
test/script_tests.cpp:1357:28: warning: ‘std::vector<CTxOut> script_tests::TxOutsFromJSON(const UniValue&)’ defined but not used [-Wunused-function]
1357 | static std::vector<CTxOut> TxOutsFromJSON(const UniValue& univalue)
| ^~~~~~~~~~~~~~
test/script_tests.cpp:1350:28: warning: ‘CMutableTransaction script_tests::TxFromHex(const string&)’ defined but not used [-Wunused-function]
1350 | static CMutableTransaction TxFromHex(const std::string& str)
| ^~~~~~~~~
...
```
This change is move-only (nice to review with `git diff --color-moved`).
ACKs for top commit:
practicalswift:
ACK 76bbcc414f: diff looks correct
fanquake:
ACK 76bbcc414f - verified that this fixes the warnings. As mentioned can be reviewed with `git diff HEAD~ --color-moved=dimmed_zebra`.
Tree-SHA512: 7799ac190d1e3f15e38b36cfcd1f8d138be80cab6c6cfad8f7828e07deffc2037d52f1d967f7f233a3a8ed74eee184f5275076c2f364c3e363c77a1f40aa5030
bd5215103e random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
Pull request description:
As shown below when resizing the read buffer `vData` `std::max((vData.size() * 3) / 2, nMaxSize)` is used. This means that the buffer size immediately jumps to `nMaxSize`. I believe the intend of this code is to grow the buffer size through several steps rather than immediately resize it to the max size.
```cpp
std::vector<unsigned char> vData(250000, 0);
long ret = 0;
unsigned long nSize = 0;
const size_t nMaxSize = 10000000; // Bail out at more than 10MB of performance data
while (true) {
nSize = vData.size();
ret = RegQueryValueExA(HKEY_PERFORMANCE_DATA, "Global", nullptr, nullptr, vData.data(), &nSize);
if (ret != ERROR_MORE_DATA || vData.size() >= nMaxSize)
break;
vData.resize(std::max((vData.size() * 3) / 2, nMaxSize)); // Grow size of buffer exponentially
}
```
vData always starts at size 250,000 and nMaxSize is always 10,000,000 so the first time this line is reached:
```cpp
vData.resize(std::max((vData.size() * 3) / 2, nMaxSize));
```
the effect will always be to resize vData to nMaxSize. Then because the loop terminates when vData.size >= 10,000,000 only one resize operation will take place.
To fix this issue we replace `std::min` with `std::max`
This PR also adds a comment clarifying the behavior of this function the first time it is called.
ACKs for top commit:
fanquake:
ACK bd5215103e - thanks for taking a look at this Ethan. Swapping from `std::max` to `std::min` here certainly seems correct.
Tree-SHA512: 7c65f700e5bbe44bc2f1ffdcdc99ec19c542894c95b5ee9791facd09d02afae88d1f8f35af129719e4860db94bc790856e7adb1d218a395381e7c2913b95f1d0
95fedd33a2 refactor: Clean up -Wlogical-op warning (maskoficarus)
Pull request description:
This is a quick patch that fixes #19912 . This change prevents a -Wlogical-op warning that occurs because we're treating a const int value as a boolean. There's no sense checking if a non-zero constant has a value, so I've removed the check.
#18836 also addresses the same warning, but has a larger scope and will require more review. This pull request will act as a patch to prevent this compile warning until 18836 is merged.
ACKs for top commit:
MarcoFalke:
review ACK 95fedd33a2
hebasto:
ACK 95fedd33a2, tested on Linux Mint 20 (x86_64):
Tree-SHA512: 13a9d7f7cb472f4c22a01ca2f9771a75267ff769bdae9d0dc6b2c7f3b05369f6dfa859be2b172b39c15ede6c44cddf556380b3565e42850faa65ccd3fe6e175b
da0988daf1 scripted-diff: rename vRecvGetData (Neha Narula)
ba951812ec Guard vRecvGetData (now in net processing) with its own mutex (Neha Narula)
2d9f2fca43 Move vRecvGetData to net processing (Neha Narula)
673247b58c Lock before checking if orphan_work_set is empty; indicate it is guarded (Neha Narula)
8803aee668 Move m_orphan_work_set to net_processing (Neha Narula)
9c47cb29f9 [Rename only] Rename orphan_work_set to m_orphan_work_set. (Neha Narula)
Pull request description:
Add annotations to guard `vRecvGetData` and `orphan_work_set` and fix up places where they were accessed without a lock. There is no current data race because they happen to be accessed by only one thread, but this might not always be the case.
Original discussion: https://github.com/bitcoin/bitcoin/pull/18861#discussion_r451778445
ACKs for top commit:
MarcoFalke:
review ACK da0988daf1🐬
jnewbery:
Code review ACK da0988daf1
hebasto:
ACK da0988daf1, I have reviewed the code and it looks correct, I agree it can be merged.
Tree-SHA512: 31cadd319ddc9273a87e77afc4db7339fd636e816b5e742eba5cb32927ac5cc07a672b2268d2d38a75a0f1b17d93836adab9acf7e52f26ea9a43f54efa57257e
51365674e8 script: Make ComputeEntrySchnorr and ComputeEntryECDSA const to clarify contract (practicalswift)
Pull request description:
Make `ComputeEntrySchnorr` and `ComputeEntryECDSA` `const` to clarify contract.
ACKs for top commit:
benthecarman:
ACK 51365674e8
theStack:
ACK 51365674e8👌
sipa:
utACK 51365674e8
Tree-SHA512: 0f7a72bf6df7a97d21045ead9db398d2a9527c358aeeb894dec34a5386da4cc316e2f3326716e960ef8aa47bf73b99d1f92bb6d45dfa7871c84624bcad8a79f1
to silence these Travis CI GCC compiler warnings:
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
too small to hold all values of ‘enum class {anonymous}::State’
State m_state : 3;
^
The warnings are based on the maximum value held by the underlying uint8_t
enumerator type, though the intention of the bitfield declaration is the
maximum declared enumerator value.
The warning been silenced in GCC 8.4+ and 9.3+ according to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414
0d9d2a1f7c Only update the updateSmartFeeLabel once in sync (Jonas Schnelli)
Pull request description:
Calling `updateSmartFeeLabel` and therefore `estimateSmartFee` is pointless during IBD.
GUI freezes appear because `estimateSmartFee` competes with `processBlock` for the `m_cs_fee_estimator` lock leading to multiple seconds of blocking the GUI thread in `updateSmartFeeLabel`.
ACKs for top commit:
ryanofsky:
Code review ACK 0d9d2a1f7c. Clever fix. Didn't test but I remember I could reproduce the startup issue easily before by putting a sleep in estimateSmartFee.
promag:
Code review ACK 0d9d2a1f7c.
hebasto:
ACK 0d9d2a1f7c, tested on Linux Mint 20 (x86_64) with `QT_FATAL_WARNINGS=1` and `-debug=qt`.
Tree-SHA512: 85ec2266f06ddd7b523e24d2a462f10ed965d5b4d479005263056f81b7fe49996e1568dafb84658af406e9202ed3bfa846d59c10bb951e0f97cee230e30fafd5
