Starting with the Clang shipped with Xcode 12, Apple has enabled
-Werror=implicit-function-declaration by default. This causes bdbs mutex
detection to fail when building on macOS (not cross-compiling):
checking for mutexes... UNIX/fcntl
configure: WARNING: NO SHARED LATCH IMPLEMENTATION FOUND FOR THIS PLATFORM.
configure: error: Unable to find a mutex implementation
as previously emitted warnings are being turned into errors. i.e:
error: implicitly declaring library function 'exit' with type 'void (int) __attribute__((noreturn))' [-Werror,-Wimplicit-function-declaration]
Append -Wno-error=implicit-function-declaration to cflags so that
-Wimplicit-function-declaration returns to being a warning, and the
configure checks will succeed.
Fixes#19411.
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
fa38093bee doc: Merge release notes (MarcoFalke)
Pull request description:
Now that all features are merged, open release notes editing at the wiki
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft
ACKs for top commit:
fanquake:
ACK fa38093bee
Tree-SHA512: ced161a2fcb0366a77a05b020c8dd65a0cf0a0de17b8260bbca9b5833ed370f92b1b81116bfc59b83380bf28b55d8963c628cf13a0cad603e5c823341b446065
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
b128b56672 test: add logging for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
8ee3536b2b test: remove unused helpers random_transaction(), make_change() and gather_inputs() (Sebastian Falbesoner)
fddce7e199 test: use MiniWallet for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_getblocktemplate_longpoll.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078. Also adds missing log messages for the subtests.
This was the only functional test that used the `random_transaction` helper in `test_framework/util.py`, hence it is removed, together with other helpers (`make_change` and `gather_inputs`) that were again only used by `random_transaction`.
ACKs for top commit:
MarcoFalke:
ACK b128b56672
Tree-SHA512: 09a5fa7b0f5976a47040f7027236d7ec0426d5a4829a082221c4b5fae294470230e89ae3df0bca0eea26833162c03980517f5cc88761ad251c3df4c4a49bca46
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
fa68755364 contrib: Fix gen_key_io_test_vectors.py imports (MarcoFalke)
Pull request description:
The script currently fails with
```
Traceback (most recent call last):
File "./gen_key_io_test_vectors.py", line 18, in <module>
from segwit_addr import bech32_encode, decode, convertbits, CHARSET
ImportError: cannot import name 'decode' from 'segwit_addr'
```
Fix that.
Also, unrelated cleanup to use the `bytearray.hex()` method instead of importing a library. https://docs.python.org/3.5/library/stdtypes.html#bytes.hex
ACKs for top commit:
theStack:
tested ACK fa68755364
Tree-SHA512: 45ff7d710de3d0ef5ac6d91543cff0edff6189d2cd00b0f8889f4361e66ef1825f12aea9e71d62038c14a7a531bfc95ffe9a1df83b85aa7f3dd666df07a6be81
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
d438d609cd QA: Use GBT to get block versions correct (Luke Dashjr)
1df2cd1c8f QA: blocktools: Accept block template to create_block (Luke Dashjr)
Pull request description:
The goal here is to decouple unrelated tests from the details of block versions.
Currently, these tests are forcing specific versions of blocks for no real reason.
ACKs for top commit:
fjahr:
re-ACK d438d609cd
benthecarman:
ACK d438d60
Tree-SHA512: 523b1cd4dac8d65c88432e126ce7f60df96ca4b94f7ecc8e83ba4ffbade23e2afe7055fdf586ce3c195a533f2004e63fff83add4267b39473a581c9f1c6d5340
1d22300b99 Address functional test nits (Pieter Wuille)
5669642a0b docs: mention BIPs 340-342 in doc/bips.md (Pieter Wuille)
Pull request description:
This addresses some nits in the tests, and adds entries for BIP 340-342 to doc/bips.md.
ACKs for top commit:
fanquake:
ACK 1d22300b99
benthecarman:
ACK 1d22300b99
Tree-SHA512: ad8f937dc6a34db86c585f65beb80e7eceda1822d9a20c86346a319908870381062856d0b95b42049a2791317a038c77fbcbf896c9f4aaa7318e4864b7fcf7a4
3333077823 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
cccc752569 rpc: Properly deserialize txs with witness before signing (MarcoFalke)
Pull request description:
Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If `try_no_witness` decoding is attempted, this will lead to rare intermittent failures.
Fixes#18803
ACKs for top commit:
achow101:
ACK 3333077823
ajtowns:
ACK 3333077823
Tree-SHA512: 73f8a5cdfe03fb0e68908d2fa09752c346406f455694a020ec0dd1267ef8f0a583b8e84063ea74aac127106dd193b72623ca6d81469a94b3f5b3c766ebf2c42b
41dca087b7 [trivial] Extract connection type doc into file where it is used. (Amiti Uttarwar)
3069b56a45 [doc] Improve help for getpeerinfo connection_type field. (Amiti Uttarwar)
Pull request description:
two commits addressing small followups from #19725
* first commit adds a clarification in the release notes that this field shouldn't be expected to be stable (suggested by sdaftuar in https://github.com/bitcoin/bitcoin/pull/19725#issuecomment-697421878)
* second commit moves the `CONNECTION_TYPE_DOC` object out of the header file to reduce the size of the binary (suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19725#discussion_r495467895, he tested and found a decrease of 10kB)
ACKs for top commit:
achow101:
ACK 41dca087b7
laanwj:
Code review ACK 41dca087b7
Tree-SHA512: a555df978b4341fbe05deeb40a8a655f0d3c5c1c0adcc1737fd2cf61b204a5a24a301ca0c2b5a3616554d4abf8c57074d22dbda5a50d8450bc22c57679424985
a490d074b3 doc: Add anchors.dat to files.md (Hennadii Stepanov)
0a85e5a7bc p2p: Try to connect to anchors once (Hennadii Stepanov)
5543c7ab28 p2p: Fix off-by-one error in fetching address loop (Hennadii Stepanov)
4170b46544 p2p: Integrate DumpAnchors() and ReadAnchors() into CConnman (Hennadii Stepanov)
bad16aff49 p2p: Add CConnman::GetCurrentBlockRelayOnlyConns() (Hennadii Stepanov)
c29272a157 p2p: Add ReadAnchors() (Hennadii Stepanov)
567008d2a0 p2p: Add DumpAnchors() (Hennadii Stepanov)
Pull request description:
This is an implementation of #17326:
- all (currently 2) outbound block-relay-only connections (#15759) are dumped to `anchors.dat` file
- on restart a node tries to connect to the addresses from `anchors.dat`
This PR prevents a type of eclipse attack when an attacker exploits a victim node restart to force it to connect to new, probably adversarial, peers.
ACKs for top commit:
jnewbery:
code review ACK a490d074b3
laanwj:
Code review ACK a490d074b3
Tree-SHA512: 0f5098a3882f2814be1aa21de308cd09e6654f4e7054b79f3cfeaf26bc02b814ca271497ed00018d199ee596a8cb9b126acee8b666a29e225b08eb2a49b02ddd
6272604bef refactor: enable -netinfo to add future networks (i2p, cjdns) (Jon Atack)
82fd40216c refactor: promote some -netinfo localvars to class members (Jon Atack)
5133fab37e cli: simplify -netinfo using getpeerinfo network field (Jon Atack)
4938a109ad rpc, test: expose CNodeStats network in RPC getpeerinfo (Jon Atack)
6df7882029 net: add peer network to CNodeStats (Jon Atack)
Pull request description:
This PR:
- builds on #19991 and #19998
- exposes peer networks via a new getpeerinfo `network` field ("ipv4", "ipv6", or "onion"), and adds functional tests
- updates -netinfo to use getpeerinfo `network` rather than detecting the peer networks client-side
- refactors -netinfo to easily add future networks
ACKs for top commit:
laanwj:
ACK 6272604bef
Tree-SHA512: 28883487585135ceaaf84ce09131f2336e3193407f2e3df0960e3f4ac340f500ab94ffecb9d06a4c49bc05e3cca4f914ea4379860bea0bd5df2f834f74616015
faad92fe1c test: Remove unused nVersion=1 in p2p tests (MarcoFalke)
Pull request description:
After commit ddefb5c0b7 nVersion is no
longer used in p2p logic when sending messages. Only when receiving
messages, but in this test no messages are received.
ACKs for top commit:
laanwj:
Code review ACK faad92fe1c
fanquake:
ACK faad92fe1c
Tree-SHA512: 9a7029187aaa5a7929a4a2199646131ff1ea72df6a855ce7022dd3bb2647dd525356dbc5e460c77007eebcdeab400a689db8cb77e8239af3b539c117a4e0d16e
5b57dc5458 RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219 RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
886be97af5 Ignore incorrectly-serialized banlist.dat entries (Pieter Wuille)
883cea7dea Restore compatibility with old CSubNet serialization (Pieter Wuille)
Pull request description:
#19628 changed CSubNet for IPv4 netmasks, using the first 4 bytes of `netmask` rather than the last 4 to store the actual mask. Unfortunately, CSubNet objects are serialized on disk in banlist.dat, breaking compatibility with existing banlists (and bringing them into an inconsistent state where entries reported in `listbanned` cannot be removed).
Fix this by reverting to the old format (just for serialization). Also add a sanity check to the deserializer so that nonsensical banlist.dat entries are ignored (which would otherwise be possible if someone added IPv4 entries after #19628 but without this PR).
Reported by Greg Maxwell.
ACKs for top commit:
laanwj:
Code review ACK 886be97af5
vasild:
ACK 886be97af
Tree-SHA512: d3fb91e8ecd933406e527187974f22770374ee2e12a233e7870363f52ecda471fb0b7bae72420e8ff6b6b1594e3037a5115984c023dbadf38f86aeaffcd681e7
27fc6a38f8 DecodeHexTx: Break out transaction decoding logic into own function (Gregory Sanders)
6020ce3c01 DecodeHexTx: Try case where txn has inputs first (Gregory Sanders)
Pull request description:
Alternative/complementary to https://github.com/bitcoin/bitcoin/pull/17773 to avoid random `decoderawtransaction` failures. Most cases this is used now is on complete transactions, especially with the uptake of PSBT.
ACKs for top commit:
ajtowns:
ACK 27fc6a38f8
achow101:
ACK 27fc6a38f8
Tree-SHA512: 0a836d7c9951bf7d2764507788dbcc871d520f1ea9b77d6b22f051f4d6224ed779aba0e4f28c5c165040095ee0c70b67080c39164d82de61b19158f7ae6fddb2
0e2a5e448f tests: dumping and minimizing of script assets data (Pieter Wuille)
4567ba034c tests: add generic qa-asset-based script verification unit test (Pieter Wuille)
f06e6d0345 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille)
3c226639eb tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille)
206fb180ec --- [TAPROOT] Tests --- (Pieter Wuille)
d7ff237f29 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille)
e9a021d7e6 Make Taproot spends standard + policy limits (Pieter Wuille)
865d2c37e2 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille)
72422ce396 Implement Tapscript script validation rules (BIP 342) (Johnson Lau)
330de894a9 Use ScriptExecutionData to pass through annex hash (Pieter Wuille)
8bbed4b7ac Implement Taproot validation (BIP 341) (Pieter Wuille)
0664f5fe1f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille)
5de246ca81 Implement Taproot signature hashing (BIP 341) (Johnson Lau)
9eb590894f Add TaggedHash function (BIP 340) (Pieter Wuille)
450d2b2371 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille)
5d62e3a68b refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille)
8bd2b4e784 refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille)
107b57df9f scripted-diff: put ECDSA in name of signature functions (Pieter Wuille)
f8c099e220 --- [TAPROOT] Refactors --- (Pieter Wuille)
Pull request description:
This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
See the list of commits [below](https://github.com/bitcoin/bitcoin/pull/19953#issuecomment-691815830). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.
This is a successor to https://github.com/bitcoin/bitcoin/pull/17977 (see discussion following [this comment](https://github.com/bitcoin/bitcoin/pull/17977#issuecomment-682285983)), and will have further changes squashed/rebased. The history of this PR can be found in #19997.
ACKs for top commit:
instagibbs:
reACK 0e2a5e448f
benthecarman:
reACK 0e2a5e4
kallewoof:
reACK 0e2a5e448f
jonasnick:
ACK 0e2a5e448f almost only looked at bip340/libsecp related code
jonatack:
ACK 0e2a5e448f modulo the last four commits (tests) that I plan to finish reviewing tomorrow
fjahr:
reACK 0e2a5e448f
achow101:
ACK 0e2a5e448f
Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e
c4a29d0a90 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky)
310b0fde04 Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow)
6c6639ac9f Include sqlite3 in documentation (Andrew Chow)
f023b7cac0 wallet: Enforce sqlite serialized threading mode (Andrew Chow)
6173269866 Set and check the sqlite user version (Andrew Chow)
9d3d2d263c Use network magic as sqlite wallet application ID (Andrew Chow)
9af5de3798 Use SQLite for descriptor wallets (Andrew Chow)
9b78f3ce8e walletutil: Wallets can also be sqlite (Andrew Chow)
ac38a87225 Determine wallet file type based on file magic (Andrew Chow)
6045f77003 Implement SQLiteDatabase::MakeBatch (Andrew Chow)
727e6b2a4e Implement SQLiteDatabase::Verify (Andrew Chow)
b4df8fdb19 Implement SQLiteDatabase::Rewrite (Andrew Chow)
010e365906 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow)
ac5c1617e7 Implement SQLiteDatabase::Backup (Andrew Chow)
f6f9cd6a64 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow)
bf90e033f4 Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow)
7aa45620e2 Add SetupSQLStatements (Andrew Chow)
6636a2608a Implement SQLiteBatch::Close (Andrew Chow)
93825352a3 Implement SQLiteDatabase::Close (Andrew Chow)
a0de83372b Implement SQLiteDatabase::Open (Andrew Chow)
3bfa0fe125 Initialize and Shutdown sqlite3 globals (Andrew Chow)
5a488b3d77 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow)
ca8b7e04ab Implement SQLiteDatabaseVersion (Andrew Chow)
7577b6e1c8 Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow)
e87df82580 Add sqlite to travis and depends (Andrew Chow)
54729f3f4e Add libsqlite3 (Andrew Chow)
Pull request description:
This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`.
For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite.
We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use.
I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier.
ACKs for top commit:
Sjors:
re-utACK c4a29d0a90
promag:
Tested ACK c4a29d0a90.
fjahr:
reACK c4a29d0a90
S3RK:
Re-review ACK c4a29d0a90
meshcollider:
re-utACK c4a29d0a90
hebasto:
re-ACK c4a29d0a90, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19077#pullrequestreview-507743699) review, verified with `git range-diff master d18892dcc c4a29d0a9`.
ryanofsky:
Code review ACK c4a29d0a90. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction.
jonatack:
ACK c4a29d0a90, debug builds and test runs after rebase to latest master @ c2c4dbaebd, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns.
Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
52380bf304 Squashed 'src/secp256k1/' changes from 8ab24e8dad..c6b6b8f1bb (Pieter Wuille)
Pull request description:
This updates the libsecp256k1 subtree to the latest master, which includes:
* Enabling the GLV endomorphism optimization by default (and removing support for the non-GLV EC multiplication)
* Added a proof for the correctness of the lambda split algorithm by roconnor-blockstream (other code was relying on the fact that it always outputs 128 bit results, which isn't at all obvious).
* Improved exhaustive tests, in particular for the Schnorr signature module
* Various other testing and CI improvements
ACKs for top commit:
fanquake:
ACK 9e5626d2a8 - performed a squash and checked that the changes were the same. The non-endomorphism code has now been ripped out.
benthecarman:
ACK 9e5626d
Tree-SHA512: 50fda5f3f934ee525f01cfc15e4f5efbc5261a97f2b77fe1b3453ee0edcf1281ad74ab4532a2fe1fe907652dd47023beff8cf3d73bf34f65ac914a694b9e7110
fa1f6f237d net: Send post-verack handshake messages at most once (MarcoFalke)
Pull request description:
There is no need to send `SENDHEADERS` and `SENDCMPCT` messages as a reply to each `VERACK` that is received. For alive checks, a `PING`/`PONG` can be used.
ACKs for top commit:
jonatack:
Concept ACK fa1f6f237d this is the only code section that sets `fCurrentlyConnected` and `fSuccessfullyConnected` to true. Could add a test. I did not verify if this code is actually being called repeatedly post initial verack; was it?
hebasto:
ACK fa1f6f237d, I have reviewed the code and it looks OK, I agree it can be merged.
naumenkogs:
ACK fa1f6f237d
laanwj:
Code review ACK fa1f6f237d
Tree-SHA512: c841d5d3807254a49463bbcfac3b32881b34a9d3206899544c86322c20988e17ad2ae243cba227fd3825a914f0cb2584451edda2414aecee6d5e3f5a0636f08a
c6b6b8f1bb Merge #830: Rip out non-endomorphism code + dependencies
c582abade1 Consistency improvements to the comments
63c6b71616 Reorder comments/function around scalar_split_lambda
2edc514c90 WNAF of lambda_split output has max size 129
4232e5b7da Rip out non-endomorphism code
ebad8414b0 Check correctness of lambda split without -DVERIFY
fe7fc1fda8 Make lambda constant accessible
9d2f2b44d8 Add tests to exercise lambda split near bounds
9aca2f7f07 Add secp256k1_split_lambda_verify
acab934d24 Detailed comments for secp256k1_scalar_split_lambda
76ed922a5f Increase precision of g1 and g2
6173839c90 Switch to our own memcmp function
63150ab4da Merge #827: Rename testrand functions to have test in name
c5257aed0b Merge #821: travis: Explicitly set --with-valgrind
bb1f54280f Merge #818: Add static assertion that uint32_t is unsigned int or wider
a45c1fa63c Rename testrand functions to have test in name
5006895bd6 Merge #808: Exhaustive test improvements + exhaustive schnorrsig tests
4eecb4d6ef travis: VALGRIND->RUN_VALGRIND to avoid confusion with WITH_VALGRIND
66a765c775 travis: Explicitly set --with-valgrind
d7838ba6a6 Merge #813: Enable configuring Valgrind support
7ceb0b7611 Merge #819: Enable -Wundef warning
8b7dcdd955 Add exhaustive test for extrakeys and schnorrsig
08d7d89299 Make pubkey parsing test whether points are in the correct subgroup
87af00b511 Abstract out challenge computation in schnorrsig
63e1b2aa7d Disable output buffering in tests_exhaustive.c
39f67dd072 Support splitting exhaustive tests across cores
e99b26fcd5 Give exhaustive_tests count and seed cmdline inputs
49e6630bca refactor: move RNG seeding to testrand
b110c106fa Change exhaustive test groups so they have a point with X=1
cec7b18a34 Select exhaustive lambda in function of order
78f6cdfaae Make the curve B constant a secp256k1_fe
d7f39ae4b6 Delete gej_is_valid_var: unused outside tests
8bcd78cd79 Make secp256k1_scalar_b32 detect overflow in scalar_low
c498366e5b Move exhaustive tests for recovery to module
be31791543 Make group order purely compile-time in exhaustive tests
e73ff30922 Enable -Wundef warning
c0041b5cfc Add static assertion that uint32_t is unsigned int or wider
4ad408faf3 Merge #782: Check if variable=yes instead of if var is set in travis.sh
412bf874d0 configure: Allow specifying --with[out]-valgrind explicitly
34debf7a6d Modify .travis.yml to explictly pass no in env vars instead of setting to nothing
a0e99fc121 Merge #814: tests: Initialize random group elements fully
5738e8622d tests: Initialize random group elements fully
c9939ba55d Merge #812: travis: run bench_schnorrsig
a51f2af62b travis: run bench_schnorrsig
ef37761fee Change travis.sh to check if variables are equal to yes instead of not-empty. Before this, setting `VALGRIND=wat` was considered as true, and to make it evaluate as false you had to unset the variable `VALGRIND=` but not it checks if `VALGRIND=yes` and if it's not `yes` then it's evaluated to false
git-subtree-dir: src/secp256k1
git-subtree-split: c6b6b8f1bb044d7d1aa065ebb674adde98a36a8e
fd9a0060f0 Report and verify expirations (Pieter Wuille)
86f50ed10f Delete limitedmap as it is unused now (Pieter Wuille)
cc16fff3e4 Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille)
173a1d2d3f Expedite removal of tx requests that are no longer needed (Pieter Wuille)
de11b0a4ef Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille)
242d16477d Change transaction request logic to use txrequest (Pieter Wuille)
5b03121d60 Add txrequest fuzz tests (Pieter Wuille)
3c7fe0e5a0 Add txrequest unit tests (Pieter Wuille)
da3b8fde03 Add txrequest module (Pieter Wuille)
Pull request description:
This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests).
The major changes are:
* Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first.
* No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available).
* The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions).
This replaces #19184, rebased on #18044 and with many small changes.
ACKs for top commit:
ariard:
Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic.
MarcoFalke:
Approach ACK fd9a0060f0🏹
naumenkogs:
Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events.
jnewbery:
utACK fd9a0060f0
jonatack:
WIP light ACK fd9a0060f0 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers.
ryanofsky:
Light code review ACK fd9a0060f0, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on:
Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7
