aboutsummaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2023-10-02rpc: getrawaddrman for addrman entries0xb10c
Exposing address manager table entries in a hidden RPC allows to introspect addrman tables in tests and during development. As response JSON object the following FORMAT1 is choosen: { "table": { "<bucket>/<position>": { "address": "..", "port": .., ... }, "<bucket>/<position>": { "address": "..", "port": .., ... }, "<bucket>/<position>": { "address": "..", "port": .., ... }, ... } } An alternative would be FORMAT2 { "table": { "bucket": { "position": { "address": "..", "port": .., ... }, "position": { "address": "..", "port": .., ... }, .. }, "bucket": { "position": { "address": "..", "port": .., ... }, .. }, } } FORMAT1 and FORMAT2 have different encodings for the location of the address in the address manager. While FORMAT2 might be easier to process for downstream tools, it also mimics internal addrman mappings, which might change at some point. Users not interested in the address location can ignore the location key. They don't have to adapt to a new RPC response format, when the internal addrman layout changes. Additionally, FORMAT1 is also slightly easier to to iterate in downstream tools. The RPC response-building implemenation complexcity is lower with FORMAT1 as we can more easily build a "<bucket>/<position>" key than a multiple "bucket" objects with multiple "position" objects (FORMAT2).
2023-10-02Merge bitcoin/bitcoin#28176: tests: add coverage to feature_addrman.pyfanquake
380130d9d70f8f8d395949a51f43806f6e600efa test: add coverage to feature_addrman.py (kevkevin) Pull request description: I added two new tests that will cover the nNew and nTried tests which add coverage to the if block by checking values larger than our range since we only check for negative values now adding coverage to these lines https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L273 https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L280 our test seem to only cover the `nTried < 0` and `nNew < 0` scenarios ACKs for top commit: ismaelsadeeq: ACK 380130d9d70f8f8d395949a51f43806f6e600efa, code looks good to me 🍃 . 0xB10C: Re-ACK 380130d9d70f8f8d395949a51f43806f6e600efa Tree-SHA512: a063bd9ca4d2d536a27c8c22a28fb13759a96f19cd8ba6cb8879cf7f65046d4ff6e8f70df17feaffd0d0d08ef914cb18a11258d313a4841c811a7e7ae4df6d5b
2023-10-02Merge bitcoin/bitcoin#28542: wallet: Check for uninitialized last processed ↵fanquake
and conflicting heights in MarkConflicted 782701ce7d31919dba2241ee43b582d8ae5a2541 test: Test loading wallets with conflicts without a chain (Andrew Chow) 4660fc82a1f5cf6eb6404d5268beef5919581661 wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) Pull request description: `MarkConflicted` assumes that `m_last_block_processed_height` is always valid. However it may not be valid when a chain is not attached, as happens in the wallet tool and during migration. In such situations, when the conflicting height is also negative (which occurs on loading when no chain is available), the calculation of the number of conflict confirms results in a non-negative value which passes the existing check for valid values. This will subsequently hit an assertion in `GetTxDepthInMainChain`. Furthermore, `MarkConflicted` is also only called on loading a transaction whose parent has a stored state of `TxStateConflicted` and was loaded before the child transaction. This depends on the loading order, which for both sqlite and bdb depends on the txids. We can avoid this by explicitly checking that both `m_last_block_processed_height` and `conflicting_height` are non-negative. Both `tool_wallet.py` and `wallet_migration.py` are updated to create wallets with a state that triggers the assertion. Fixes #28510 ACKs for top commit: ryanofsky: Code review ACK 782701ce7d31919dba2241ee43b582d8ae5a2541. Nice catch, and clever test (grinding the txid) furszy: ACK 782701ce Tree-SHA512: 1344e0279ec5413a43a2819d101fb571fbf4821de2d13958a0fdffc99f57082ef3243ec454c8343f97dc02ed1fce8c8b0fd89388420ab2e55618af42ad5630a9
2023-10-02Merge bitcoin/bitcoin#28556: doc: fix link to developer-notes.md file in ↵fanquake
multiprocess.md d9b172cd00fc3a8de1308e4469b82f5da474ea33 doc: fix link to developer-notes.md file in multiprocess.md (David Álvarez Rosa) Pull request description: Fix link to `developer-notes.md` file in `multiprocess.md`. ACKs for top commit: fanquake: ACK d9b172cd00fc3a8de1308e4469b82f5da474ea33 Tree-SHA512: 55fffefb37c4d67acb1fa8b0660216ec1c7f2c2314d11e4d319cae40480ed59ef448909fa2ca334167c86d60d41932220dce4186e28fa300f4d03eb0b3c769d0
2023-10-02Merge bitcoin/bitcoin#28508: refactor: Remove SER_GETHASH, hard-code client ↵fanquake
version in CKeyPool serialize fac29a0ab19fda457b55d7a0a37c5cd3d9680f82 Remove SER_GETHASH, hard-code client version in CKeyPool serialize (MarcoFalke) fa72f09d6ff8ee204f331a69d3f5e825223c9e11 Remove CHashWriter type (MarcoFalke) fa4a9c0f4334678fb80358ead667807bf2a0a153 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader (MarcoFalke) Pull request description: Removes a bunch of redundant, dead or duplicate code. Uses the idea from and finishes the idea https://github.com/bitcoin/bitcoin/pull/28428 by theuni ACKs for top commit: ajtowns: ACK fac29a0ab19fda457b55d7a0a37c5cd3d9680f82 kevkevinpal: added one nit but otherwise ACK [fac29a0](https://github.com/bitcoin/bitcoin/pull/28508/commits/fac29a0ab19fda457b55d7a0a37c5cd3d9680f82) Tree-SHA512: cc805e2f38e73869a6691fdb5da09fa48524506b87fc93f05d32c336ad3033425a2d7608e317decd3141fde3f084403b8de280396c0c39132336fe0f7510af9e
2023-10-02Merge bitcoin/bitcoin#28500: Prevent default/invalid CKey objects from ↵fanquake
allocating secure memory 6ef405ddb195bbf1b28a906d8c8bb877f0c17d7b key: don't allocate secure mem for null (invalid) key (Pieter Wuille) d9841a7ac634472c1a9105f81f8e7b55e4bd1a4a Add make_secure_unique helper (Anthony Towns) Pull request description: Bitcoin Core has `secure_allocator`, which allocates inside special "secure" (non-swappable) memory pages, which may be limited in availability. Currently, every `CKey` object uses 32 such secure bytes, even when the `CKey` object contains the (invalid) value zero. Change this to not use memory when the `CKey` is invalid. This is particularly relevant for `BIP324Cipher` which briefly holds a `CKey`, but after receiving the remote's public key and initializing the encryption ciphers, the key is wiped. In case secure memory usage is in high demand, it'd be silly to waste it on P2P encryption keys instead of wallet keys. ACKs for top commit: ajtowns: ACK 6ef405ddb195bbf1b28a906d8c8bb877f0c17d7b john-moffett: ACK 6ef405ddb195bbf1b28a906d8c8bb877f0c17d7b Tree-SHA512: 987f4376ed825daf034ea4d7c4b4952fe664b25b48f1c09fbcfa6257a40b06c4da7c2caaafa35c346c86bdf298ae21f16c68ea4b1039836990d1a205de2034fd
2023-10-02doc: fix link to developer-notes.md file in multiprocess.mdDavid Álvarez Rosa
2023-10-02Merge bitcoin/bitcoin#28227: test: check for specific bip157 disconnect ↵fanquake
reasons, add test coverage 2ab7952bda8d15e91b03f8307839030cbb55614e test: add bip157 coverage for (start height > stop height) disconnect (Sebastian Falbesoner) 63e90e1d3f5ed08f9871f07667d389ec66aa621c test: check for specific disconnect reasons in p2p_blockfilters.py (Sebastian Falbesoner) Pull request description: This PR checks for specific disconnect reasons using `assert_debug_log` in the functional test `p2p_blockfilters.py`. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message. Also, based on that, missing coverage for the (start height > stop height) disconnect case is added: https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/src/net_processing.cpp#L3050-L3056 ACKs for top commit: MarcoFalke: lgtm ACK 2ab7952bda8d15e91b03f8307839030cbb55614e furszy: Looks good, code ACK 2ab7952b Tree-SHA512: 0581cb569d5935aaa004a95a6f16eeafe628b9d816ebb89232f2832e377049df878a1e74c369fb46931b94e1a3a5e3f4aaa21a007c0a488f4ad2cda0919c605d
2023-10-02Merge bitcoin/bitcoin#28184: lint: fix custom mypy cache dir settingfanquake
f9047771d642c5887c752872b6ffbbd974603b35 lint: fix custom mypy cache dir setting (Fabian Jahr) Pull request description: fixes #28183 The custom cache dir for `mypy` can only be set via an environment variable, setting the `MYPY_CACHE_DIR` variable in the program is not sufficient. This error was introduced while translating the shell script to python. See also the mypy documentation: https://mypy.readthedocs.io/en/stable/config_file.html#confval-cache_dir ACKs for top commit: MarcoFalke: lgtm ACK f9047771d642c5887c752872b6ffbbd974603b35 Tree-SHA512: 7e8fb0cd06688129bd46d1afb8647262eb53d0f60b1ef6f288fedaa122d906fb62c9855e8bb0d6c6297d41a87a47d3cec7a00df55a7d033947937dfe23d07ba7
2023-10-02Merge bitcoin/bitcoin#28545: test: Avoid test failure on Linux root without ↵fanquake
cap-add LINUX_IMMUTABLE fa40b3ee22e78f58d7426dbc4343472ba40081e3 test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE (MarcoFalke) Pull request description: This turns a test failure on Linux when running the test as `root`, but without the `LINUX_IMMUTABLE` capability, into an early return, with a suggestion to turn on `LINUX_IMMUTABLE` next time (if possible). ACKs for top commit: pinheadmz: utACK fa40b3ee22e78f58d7426dbc4343472ba40081e3 jonatack: ACK fa40b3ee22e78f58d7426dbc4343472ba40081e3 Tree-SHA512: d986ff8aeae5f8267c21a23d5be16f7c5a4d4d3be045a6999d8b39c7b8672cfe915dedde762cc9965cdc4970940bffc4b0d1412833d8036d4425450eb6181f67
2023-10-01test: add coverage to feature_addrman.pykevkevin
I added two new tests that will cover the nNew and nTried tests which add coverage to the if block by checking values larger than our range since we only check for negative values now Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>
2023-09-29Merge bitcoin/bitcoin#27653: test: add unit test coverage for Python ECDSA ↵Andrew Chow
implementation 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983 test: add unit test coverage for Python ECDSA implementation (Sebastian Falbesoner) Pull request description: This PR adds missing unit test coverage for the Python ECDSA implementation, which should be useful for detecting potential problems early whenever changes in the test framework's Python implementation of secp256k1 are made (e.g. #26222). Note that right now we don't call `ECPubKey.verify_ecdsa` anywhere in our tests, so we wouldn't notice if it is broken at some point. To keep it simple, the already existing unit test for Schnorr signatures is extended to also check ECDSA signatures. For that purpose, the dictionary storing private-key/public-key entries use their legacy types `ECKey/ECPubKey` instead of bare byte-arrays, and for Schnorr signing/verification the necessary conversions (ECKey -> bare private key, ECPubKey -> x-only pubkey) is done later when needed. To avoid code duplication, a helper function `random_bitflip` for damaging signatures is introduced. The unit test can be run by either calling it for this single module: `$ python3 -m unittest ./test/functional/test_framework/key.py` or simply running `$ ./test/functional/test_runner.py` which calls all test framework module's unit tests at the start (see TEST_FRAMEWORK_MODULES list). ACKs for top commit: achow101: ACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983 sipa: utACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983 stratospher: tested ACK 96b3f2d. Tree-SHA512: b993f25b843fa047376addda4ce4b0f15750ffba926528b5cca4c5f99b9af456206f4e8af885d25a017dddddf382ddebf38765819b3d16a3f28810d03b010808
2023-09-29Merge bitcoin/bitcoin#27866: blockstorage: Return on fatal flush errorsRyan Ofsky
d8041d4e042957660827313951b18c8dd9a99a16 blockstorage: Return on fatal undo file flush error (TheCharlatan) f0207e00303a1030eca795ede231e3c0d94df061 blockstorage: Return on fatal block file flush error (TheCharlatan) 5671c15f4520c6dc20e0805fd0b06157ff94bcd7 blockstorage: Mark FindBlockPos as nodiscard (TheCharlatan) Pull request description: The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site. Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add `[[nodiscard]]` annotations to them such that they are not ignored in future. Functions that call either `FlushUndoFile` or `FlushBlockFile`, need to handle these extra abort cases properly. Since `Chainstate::FlushStateToDisk` already produces an abort error in case of `WriteBlockIndexDB` failing, no extra logic for functions calling `Chainstate::FlushStateToDisk` is required. Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called by `FindBlockPos`, while `FlushUndoFile` is only called by `FlushBlockFile` and `WriteUndoDataForBlock`. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases. --- This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site. Further work for enforcing returns when `AbortNode` is called is done in https://github.com/bitcoin/bitcoin/pull/27862. ACKs for top commit: stickies-v: re-ACK d8041d4 ryanofsky: Code review ACK d8041d4e042957660827313951b18c8dd9a99a16 Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219
2023-09-29Merge bitcoin/bitcoin#28525: net: Drop v2 garbage authentication packetAndrew Chow
e3720bca398820038b3e97f467adb2c45ef9ef5f net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing) b0f5175c044df956c0f07f540706d457c4912856 net: Drop v2 garbage authentication packet (Tim Ruffing) Pull request description: Note that this is a breaking change, see also https://github.com/bitcoin/bips/pull/1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing. ACKs for top commit: naumenkogs: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f sipa: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f. Re-ran the v2 transport fuzzer overnight. ajtowns: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f - simpler and more flexible, nice achow101: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f Sjors: utACK e3720bca398820038b3e97f467adb2c45ef9ef5f theStack: Code-review ACK e3720bca398820038b3e97f467adb2c45ef9ef5f Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
2023-09-28Merge bitcoin/bitcoin#28540: tests: Fix wallet_resendwallettransactions.py ↵Andrew Chow
intermittent failure by using manual bumps instead of bumpfee b5a962564eb15075e4e2a7bc0c235a56fa998ac3 tests: Use manual bumps instead of bumpfee for resendwallettransactions (Andrew Chow) Pull request description: Bumpfee will try to increase the entire package to the target feerate, which causes repeated bumpfees to quickly shoot up in fees, causing intermittent failures when the fee is too large. We don't care about this property, just that the child is continuously replaced until we observe it's position in mapWallet is before its parent. Instead of using bumpfee, we can create raw transactions which have only pay (just above) the additional incremental relay fee, thus avoiding this problem. Fixes #28491 ACKs for top commit: kevkevinpal: ACK [b5a9625](https://github.com/bitcoin/bitcoin/pull/28540/commits/b5a962564eb15075e4e2a7bc0c235a56fa998ac3) mzumsande: Code review ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3 pablomartin4btc: ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3 -> adding the `try_rpc` to avoid (skip) any possible failure around the manual bump fee (if we ever reach it as [explained](https://github.com/bitcoin/bitcoin/pull/28540#issuecomment-1737648048)) makes a lot of sense as the spirit of the test is the tx (child before parent) sort in the `mapWallet` (as also [explained](https://github.com/bitcoin/bitcoin/issues/28491#issuecomment-1736161363)). MarcoFalke: lgtm ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3 Tree-SHA512: f184f11c73be0c30753181901f51a3b4b9c4135e0c4681e9f4ca94692c49bac15c91683c85266a2124333c8593e9919bfd9102724616faab299740f2eb98741f
2023-09-28lint: fix custom mypy cache dir settingFabian Jahr
2023-09-28Merge bitcoin/bitcoin#28450: Add package evaluation fuzzerglozow
262ab8ef7860d43cebc9d04721e3a075b4edf06e Add package evaluation fuzzer (Greg Sanders) Pull request description: This fuzzer target caught the issue in https://github.com/bitcoin/bitcoin/pull/28251 within 5 minutes on master branch, and an additional issue which I've applied a preliminary patch to cover. Fuzzer target does the following: 1) Picks mempool confgs, including max package size, count, mempool size, etc 2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints 3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool 4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies 5) Otherwise submits through package submission interface 6) Repeat 1-5 a few hundred times per mempool instance In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master. The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I'll continue working on re-adding these assertions for further invariant testing. ACKs for top commit: murchandamus: ACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e glozow: reACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e dergoegge: tACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5
2023-09-27Merge bitcoin/bitcoin#28505: rpc: bumpfee, improve doc for 'reduce_output' argAndrew Chow
b3db8c9d5ccfe5c31341169fa7ac044427122921 rpc: bumpfee, improve doc for 'reduce_output' arg (furszy) Pull request description: Fixes #28180. Resulted from discussions with S3RK, achow101, and Murch. The current argument name and description are dangerous as it don't describe the case where the user selects the recipient output as the change address. This one could end up been increased by the inputs minus outputs remainder. Which, when `bumpfee` adds new inputs to the transaction, leads the process to send more coins to the recipient. Which is not what the user would expect from a 'reduce_output' param naming. ACKs for top commit: S3RK: ACK b3db8c9d5ccfe5c31341169fa7ac044427122921 achow101: ACK b3db8c9d5ccfe5c31341169fa7ac044427122921 murchandamus: ACK b3db8c9d5ccfe5c31341169fa7ac044427122921 Tree-SHA512: 91f607e2f5849041d7c099afdddae11af8bed5b1ac90c9d22921267f272e21b44e107d6968e037f05f958a61fe29e94e5fb44b224fb3606f197f83ec4ba3b1e7
2023-09-27Add package evaluation fuzzerGreg Sanders
2023-09-27key: don't allocate secure mem for null (invalid) keyPieter Wuille
Instead of storing the key material as an std::vector (with secure allocator), use a secure_unique_ptr to a 32-byte array, and use nullptr for invalid keys. This means a smaller CKey type, and no secure/dynamic memory usage for invalid keys.
2023-09-27Add make_secure_unique helperAnthony Towns
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
2023-09-27tests: Use manual bumps instead of bumpfee for resendwallettransactionsAndrew Chow
Bumpfee will try to increase the entire package to the target feerate, which causes repeated bumpfees to quickly shoot up in fees, causing intermittent failures when the fee is too large. We don't care about this property, just that the child is continuously replaced until we observe it's position in mapWallet is before its parent. Instead of using bumpfee, we can create raw transactions which have only pay the additional incremental relay fee, thus avoiding this problem.
2023-09-27test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLEMarcoFalke
2023-09-27net: Simplify v2 recv logic by decoupling AAD from state machineTim Ruffing
2023-09-27net: Drop v2 garbage authentication packetTim Ruffing
See also https://github.com/bitcoin/bips/pull/1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing.
2023-09-26test: Test loading wallets with conflicts without a chainAndrew Chow
Loading a wallet with conflicts without a chain (e.g. wallet tool and migration) would previously result in an assertion due to -1 being both a valid number of conflict confirmations, and the indicator that that member has not been set yet.
2023-09-26wallet: Check last block and conflict height are valid in MarkConflictedAndrew Chow
MarkConflicted calculates conflict confirmations incorrectly when both the last block processed height and the conflicting height are negative (i.e. uninitialized). If either are negative, we should not be marking conflicts and should exit early.
2023-09-26rpc: bumpfee, improve doc for 'reduce_output' argfurszy
The current argument name and description are dangerous as it don't describe the case where the user selects the recipient output as the change address. This one could end up been increased by the inputs minus outputs remainder. Which, when bumpfee adds new inputs to the transaction, leads the process to send more coins to the recipient. Which is not what the user would expect from a 'reduce_output' param naming. Co-authored-by: Murch <murch@murch.one>
2023-09-26Merge bitcoin/bitcoin#28483: refactor: Return CAutoFile from ↵fanquake
BlockManager::Open*File() fa56c421be04af846f479c30749b17e6663ab418 Return CAutoFile from BlockManager::Open*File() (MarcoFalke) 9999b89cd37fb2a23c5ebcd91d9cb31d69375933 Make BufferedFile to be a CAutoFile wrapper (MarcoFalke) fa389d902fbf5fac19fba8c5d0c5e0a25f15ca63 refactor: Drop unused fclose() from BufferedFile (MarcoFalke) Pull request description: This is required for https://github.com/bitcoin/bitcoin/pull/28052, but makes sense on its own, because offloading logic to `CAutoFile` instead of re-implementing it allows to delete code and complexity. ACKs for top commit: TheCharlatan: Re-ACK fa56c421be04af846f479c30749b17e6663ab418 willcl-ark: tACK fa56c421be Tree-SHA512: fe4638f3a6bd3f9d968cfb9ae3259c9d6cd278fe2912cbc90289851311c8c781099db4c160e775960975c4739098d9af801a8d2d12603f371f8edfe134d8f85a
2023-09-24Merge bitcoin/bitcoin#28512: doc: Be vague instead of wrong about ↵fanquake
MALLOC_ARENA_MAX 12f7257b8f6ba0de1fb05b598e916c8b14bcab8c doc: Be vague instead of wrong about MALLOC_ARENA_MAX (Tim Ruffing) Pull request description: Before this commit, we claim that glibc's malloc implementation uses 2 arenas by default. But that's true only on 32-bit systems, and even there, it uses *up* to 2 arenas. This commit fixes the wrong statement. The new statement is intentionally vague to reduce our maintenance burden. For details, see: https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html#index-glibc_002emalloc_002earena_005fmax Noticed in: https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1728103427 ACKs for top commit: fanquake: ACK 12f7257b8f6ba0de1fb05b598e916c8b14bcab8c Tree-SHA512: c0ff1e35b682a841e366a1cad26e18ff79a93d97103529be35a972c7dcbb95f5354e7a7b98a86731f491434d64685bb58cc3cc9100f0577d8f75db05e951b09a
2023-09-23Merge bitcoin/bitcoin#28385: [refactor] rewrite ↵fanquake
DisconnectedBlockTransactions to not use boost 4313c77400eb8eaa8586db39a7e29a861772ea80 make DisconnectedBlockTransactions responsible for its own memory management (glozow) cf5f1faa037e9a40a5029cc7dd4ee61454b62466 MOVEONLY: DisconnectedBlockTransactions to its own file (glozow) 2765d6f3434c101fe2d46e9313e540aa680fbd77 rewrite DisconnectedBlockTransactions as a list + map (glozow) 79ce9f0aa46de8ff742be83fd6f68eab40e073ec add std::list to memusage (glozow) 59a35a7398f5bcb3e3805d1e4f363e4c2fb336b3 [bench] DisconnectedBlockTransactions (glozow) 925bb723ca71aa76380b769d8926c7c2ad9bbb7b [refactor] batch-add transactions to DisconnectedBlockTransactions (glozow) Pull request description: Motivation - I think it's preferable to use stdlib data structures instead of depending on boost if we can achieve the same thing. - Also see #28335 for further context/motivation. This PR simplifies that one. Things done in this PR: - Add a bench for `DisconnectedBlockTransactions` where we reorg and the new chain has {100%, 90%, 10%} of the same transactions. AFAIU in practice, it's usually close to 100%. - Rewrite `DisconnectedBlockTransactions` as a `std::list` + `unordered_map` instead of a boost multi index container. - On my machine, the bench suggests the performance is very similar. - Move `DisconnectedBlockTransactions` from txmempool.h to its own kernel/disconnected_transactions.h. This struct isn't used by txmempool and doesn't have much to do with txmempool. My guess is that it's been living there for convenience since the boost includes are there. ACKs for top commit: ismaelsadeeq: Tested ACK 4313c77400eb8eaa8586db39a7e29a861772ea80 stickies-v: ACK 4313c77400eb8eaa8586db39a7e29a861772ea80 TheCharlatan: ACK 4313c77400eb8eaa8586db39a7e29a861772ea80 Tree-SHA512: 273c80866bf3acd39b2a039dc082b7719d2d82e0940e1eb6c402f1c0992e997256722b85c7e310c9811238a770cfbdeb122ea4babbc23835d17128f214a1ef9e
2023-09-23Merge bitcoin/bitcoin#28492: RPC: `descriptorprocesspsbt` returns hex ↵Andrew Chow
encoded tx if complete a99e9e655a58b2364a74aec5cafb827a73c6b0c4 doc: add release note (ismaelsadeeq) 2b4edf889a4b555c8c7f6793fa5d820e5513ecac test: check `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq) c405207a18fdee75a4dea470bb0d13e59e15ce45 rpc: `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq) Pull request description: Coming from [#28414 comment](https://github.com/bitcoin/bitcoin/pull/28414#pullrequestreview-1618684391) Same thing also for `descriptorprocesspsbt`. Before this PR `descriptorprocesspsbt` returns a boolean `complete` which indicates that the psbt is final, users then have to call `finalizepsbt` to get the hex encoded network transaction. In this PR if the psbt is complete the return object also has the hex encoded network transaction ready for broadcast with `sendrawtransaction`. This save users calling `finalizepsbt` with the descriptor, if it is already complete. ACKs for top commit: achow101: ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4 pinheadmz: ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4 ishaanam: ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4 Tree-SHA512: c3f1b1391d4df05216c463127cd593f8703840430a99febb54890bc66fadabf9d9530860605f347ec54c1694019173247a0e7a9eb879d3cbb420f9e8d9839b75
2023-09-22Merge bitcoin-core/gui#119: Replace send-to-self with dual send+receive entriesHennadii Stepanov
099dbe4224e0e896604e7f6901d0fc302b0bd3a0 GUI: TransactionRecord: When time/index/etc match, sort send before receive (Luke Dashjr) 2d182f77cd8100395cf47a721bd01dc8620c9718 Bugfix: Ignore ischange flag when we're not the sender (Luke Dashjr) 71fbdb7f403e673877be94a79cd4c6b13b0bbcd6 GUI: Remove SendToSelf TransactionRecord type (Luke Dashjr) f3fbe99fcf90daec79d49fd5d868102dc99feb23 GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs (Luke Dashjr) b9765ba1d67d7b74c17f9ce70cad5487715208a0 GUI: TransactionRecord: Use "any from me" as the criteria for deciding whether a transaction is a send or receive (Luke Dashjr) Pull request description: Makes the GUI transaction list more like the RPC, and IMO clearer in general. As a side effect, this also fixes the GUI entries when a transaction is a net profit to us, but some inputs were also from us. Originally https://github.com/bitcoin/bitcoin/pull/15115 Has Concept ACKs from @*Empact @*jonasschnelli ACKs for top commit: hebasto: ACK 099dbe4224e0e896604e7f6901d0fc302b0bd3a0. Tree-SHA512: 7d581add2f59431aa019126d54232a1f15723def5147d7a1b672e9b6d525b6e5a944cc437701aa1bd5bd0fbe557a3d1f4b239337f42bdba4fe1d3960442d0e3b
2023-09-22Merge bitcoin-core/gui#739: Disable and uncheck blank when private keys are ↵Hennadii Stepanov
disabled 9ea31eba04ff8dcb9d7d993ce28bb10731a35177 gui: Disable and uncheck blank when private keys are disabled (Andrew Chow) Pull request description: Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled. ACKs for top commit: S3RK: Code review ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177 jarolrod: ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177 pablomartin4btc: tACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177 Tree-SHA512: 0c90dbd77e66f088c6a57711a4b91e254814c4ee301ab703807f281cacd4b08712d2dfeac7661f28bc0e93acc55d486a17b8b4a53ffa57093d992e7a3c51f4e8
2023-09-22Merge bitcoin-core/gui#755: Silence `-Wcast-function-type` warningHennadii Stepanov
befb42f1462f886bf5bed562ba1dae00853cecde qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov) Pull request description: On Fedora 38 @ 8f7b9eb8711fdb32e8bdb59d2a7462a46c7a8086: ``` $ x86_64-w64-mingw32-g++ --version | head -1 x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38) $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type" $ make > /dev/null qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)': qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type] 46 | PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` [Required](https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1713999563) for https://github.com/bitcoin/bitcoin/pull/25972. Picked from https://trac.nginx.org/nginx/ticket/1865. ACKs for top commit: MarcoFalke: review ACK befb42f1462f886bf5bed562ba1dae00853cecde Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
2023-09-21Merge bitcoin/bitcoin#28513: ci: Install Homebrew's `pkg-config` packagefanquake
43cd8029fa39e0bd4bf6fb896952952bcae16160 ci: Install Homebrew's `pkg-config` package (Hennadii Stepanov) Pull request description: Some versions of macOS images lack the `pkg-config` package. For example, https://github.com/bitcoin/bitcoin/actions/runs/6248032071/job/16961797066: ``` Runner Image Image: macos-13 Version: 20230417.1 ``` ``` + ./autogen.sh configure.ac:16: error: PKG_PROG_PKG_CONFIG macro not found. Please install pkg-config and re-run autogen.sh ``` This PR makes Homebrew install the `pkg-config` package explicitly. Also please refer to [macOS Build Guide](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md). ACKs for top commit: kevkevinpal: ACK [43cd802](https://github.com/bitcoin/bitcoin/pull/28513/commits/43cd8029fa39e0bd4bf6fb896952952bcae16160) MarcoFalke: lgtm ACK 43cd8029fa39e0bd4bf6fb896952952bcae16160 RandyMcMillan: ACK 43cd802 Tree-SHA512: 2b934b22e5f6748634089e0525b92219484e37b2afc11e9fd4c53faf112b33ca1c8deb5b4aa36939bf5c4807e7599d4aabae6335317ecc5d4a4d562bbd7dbdf2
2023-09-21Merge bitcoin/bitcoin#28379: Refactor: Remove m_is_test_chainfanquake
78c2707b2aefa9e0aee5ddceaeab31997085a241 Refactor: Replace 'isMockableChain' with inline 'ChainType' check for 'submitpackage' (Tim Neubauer) 27b4084e16a1cb210ce27119416ee34625781052 Refactor: Remove m_is_test_chain (Tim Neubauer) Pull request description: Remove the m_is_test_chain bool Compiled and run tests locally #28376 ACKs for top commit: MarcoFalke: re-ACK 78c2707b2aefa9e0aee5ddceaeab31997085a241 ajtowns: ACK 78c2707b2aefa9e0aee5ddceaeab31997085a241 Tree-SHA512: 2eedd855c379dd12b7ff28b0e03414680cc892313f16502f36e09906513df9c222e8cc2cea3ff4d9a4f47c9efdfa00d017f38398021b0c96d4543711206d6ff8
2023-09-21Merge bitcoin/bitcoin#28471: Fix virtual size limit enforcement in ↵Andrew Chow
transaction package context eb8f58f5e4a249d55338304099e8238896d2316d Add functional test to catch too large vsize packages (Greg Sanders) 1a579f9d01256b0b2570168496d129a8b2009b35 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders) bc013fe8e3d0bae2ab766a8248219aa3c9e344df Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr) 533660c58ad5a218671a9bc1537299b1d67bb55d Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders) Pull request description: (Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to: 1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion. 2) pass correct vsize to chain limit evaluations in package context 3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits) This should fix the known issues while not blocking additional refactoring later. ACKs for top commit: achow101: ACK eb8f58f5e4a249d55338304099e8238896d2316d ariard: Re-Code ACK eb8f58f5e glozow: reACK eb8f58f5e4a249d55338304099e8238896d2316d Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39
2023-09-21Merge bitcoin/bitcoin#27934: test: added coverage to estimatefeefanquake
d05be124dbc0b24fb69d0c28ba2d6b297d243751 test: added coverage to estimatefee (kevkevin) Pull request description: Added a assert for an rpc error when we try to estimate fee for the max conf_target Line I am adding coverage to https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#LL71C52-L71C52 ACKs for top commit: MarcoFalke: lgtm ACK d05be124dbc0b24fb69d0c28ba2d6b297d243751 Tree-SHA512: dfab075989446e33d1a5ff1a308f1ba1b9f80cce3848fbe4231f69212ceef456a3f2b19365a42123e0397c31893fd9f1fd9973cc00cfbb324386e12ed0e6bccc
2023-09-21Merge bitcoin/bitcoin#28078: net, refactor: remove unneeded exports, use ↵Andrew Chow
helpers over low-level code, use optional 4ecfd3eaf434d868455466e001adae4b68515ab8 Inline short, often-called, rarely-changed basic CNetAddr getters (Jon Atack) 5316ae5dd8d90623f9bb883bb253fa6463ee4d34 Convert GetLocal() to std::optional and remove out-param (Jon Atack) f1304db136bbeac70b52522b5a4522165bfb3fc0 Use higher-level CNetAddr and CNode helpers in net.cpp (Jon Atack) 07f589158835151a7613e4b2a508c0dd61a18fd7 Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet() (Jon Atack) df488563b280c63f5d74d5ac0fcb1a2cad489d55 GetLocal() type-safety, naming, const, and formatting cleanups (stickies-v) fb4265747c9eb022d80c6f2988e574c689130348 Add and use CNetAddr::HasCJDNSPrefix() helper (Jon Atack) 5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0 Move GetLocal() declaration from header to implementation (Jon Atack) 11426f6557ac09489d5e13bf3b9d94fbd5073c8e Move CaptureMessageToFile() declaration from header to implementation (Jon Atack) deccf1c4848620cfff2a9642c5a6b5acdfbc33bd Move IsPeerAddrLocalGood() declaration from header to implementation (Jon Atack) Pull request description: and other improvements noticed while reviewing #27411. Addresses https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263969104 and https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263967598. See commit messages for details. ACKs for top commit: achow101: ACK 4ecfd3eaf434d868455466e001adae4b68515ab8 vasild: ACK 4ecfd3eaf434d868455466e001adae4b68515ab8 stickies-v: ACK 4ecfd3eaf434d868455466e001adae4b68515ab8 Tree-SHA512: d792bb2cb24690aeae9bedf97e92b64fb6fd080c39385a4bdb8ed05f37f3134d85bda99da025490829c03017fd56382afe7951cdd039aede1c08ba98fb1aa7f9
2023-09-21Merge bitcoin/bitcoin#26366: rpc, test: `addnode` improv + add test coverage ↵Andrew Chow
for invalid command f52cb02f700b58bca921a7aa24bfeee04760262b doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg) effd1efefb53c58f0e43fec4f019a19f97795553 test: `addnode` with an invalid command should throw an error (brunoerg) 56b27b84877376ffc32b3bad09f1047b23de4ba1 rpc, refactor: clean-up `addnode` (brunoerg) Pull request description: This PR: - Adds test coverage for an invalid `command` in `addnode`. - Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones. - Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id. - Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field. ACKs for top commit: achow101: ACK f52cb02f700b58bca921a7aa24bfeee04760262b jonatack: ACK f52cb02f700b58bca921a7aa24bfeee04760262b theStack: re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e
2023-09-20Merge bitcoin-core/gui#738: Add menu option to migrate a walletHennadii Stepanov
48aae2cffeb91add75a70ac4d5075c38054452fa gui: Add File > Migrate Wallet (Andrew Chow) 577be889cd52fc2d896a5f39c66bc2cadb8622e4 gui: Optionally return passphrase after unlocking (Andrew Chow) 5b3a85b4c6ffd1f29a917d4c1af4bff6c0ea2ef5 interfaces, wallet: Expose migrate wallet (Andrew Chow) Pull request description: GUI users need to be able to migrate wallets without going to the RPC console. ACKs for top commit: jarolrod: ACK 48aae2cffeb91add75a70ac4d5075c38054452fa pablomartin4btc: tACK 48aae2cffeb91add75a70ac4d5075c38054452fa hebasto: ACK 48aae2cffeb91add75a70ac4d5075c38054452fa Tree-SHA512: 2d02b1e85e7d6cfbf503f417f150cdaa0c63822942e9a6fe28c0ad3e7f40a957bb01a375c909a60432dc600e84574881aa446c7ec983b56f0bb23f07ef15de54
2023-09-20ci: Install Homebrew's `pkg-config` packageHennadii Stepanov
Some versions of macOS images lack the 'pkg-config' package, which is required for the build process.
2023-09-20Merge bitcoin/bitcoin#28154: test: refactor: deduplicate segwitv0 ECDSA ↵Andrew Chow
signing for tx inputs 83d7cfd5429b0be7755bc48145032956f5f56dae test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs (Sebastian Falbesoner) Pull request description: This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature: 1. calculate the `SegwitV0SignatureHash` with the desired sighash type 2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above 3. put the DER-encoded result (plus sighash byte) at the bottom of the witness stack ACKs for top commit: achow101: ACK 83d7cfd5429b0be7755bc48145032956f5f56dae pinheadmz: code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae Tree-SHA512: b8e55409ddc9ddb14cfc06daeb4730d7750a4632f175f88dcac6ec4d216e71fd4a7eee325a64d6ebba3b33be50bcd30c2de7400f834c01abb67e52840d9823b6
2023-09-20doc: Be vague instead of wrong about MALLOC_ARENA_MAXTim Ruffing
Before this commit, we claim that glibc's malloc implementation uses 2 arenas by default. But that's true only on 32-bit systems, and even there, it uses *up* to 2 arenas. This commit fixes the wrong statement. The new statement is intentionally vague to reduce our maintenance burden. For details, see: https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html#index-glibc_002emalloc_002earena_005fmax Noticed in: https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1728103427
2023-09-20test: add unit test coverage for Python ECDSA implementationSebastian Falbesoner
2023-09-20Merge bitcoin/bitcoin#28504: ci: Use nproc over MAKEJOBS in 01_base_installfanquake
fa3b5e5e57cd4649d577e06a3aca798b55a75fd5 ci: Use nproc over MAKEJOBS in 01_base_install (MarcoFalke) Pull request description: Currently `$MAKEJOBS` is the default value in `01_base_install.sh` when building the container image. This problem can't be fixed (see below), so just use `nproc` for now. Other solutions would be bad: * Passing in the `MAKEJOBS` as a dockerfile env would create a new image if the number of tasks are changed, seems verbose and confusing. * Leaving `master` as-is would leave CPUs unused if there are more than `4`. ACKs for top commit: fanquake: ACK - I think this is fine as-is, it's an improvement over the current state fa3b5e5e57cd4649d577e06a3aca798b55a75fd5 Tree-SHA512: 15014eb7b548945737b0fed5cd46f0cd4b72b1d54d044f60fce628f914053a2de6518878428a54822d236d08d6f257d8c464d3fb589400f4be56022d2ec8676d
2023-09-20Add functional test to catch too large vsize packagesGreg Sanders
2023-09-20Handle over-sized (in virtual bytes) packages with no in-mempool ancestorsGreg Sanders
2023-09-20Merge bitcoin/bitcoin#27511: rpc: Add test-only RPC getaddrmaninfo for ↵Andrew Chow
new/tried table address count 28bac81a346c0b68273fa73af924f7096cb3f41d test: add functional test for getaddrmaninfo (stratospher) c8eb8dae51039aa1938e7040001a149210e87275 rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher) Pull request description: implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate. This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman. ```jsx $ getaddrmaninfo Result: { (json object) json object with network type as keys "network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns) "new" : n, (numeric) number of addresses in new table "tried" : n, (numeric) number of addresses in tried table "total" : n (numeric) total number of addresses in both new/tried tables from a network }, ... } ``` ### additional context from [original PR](https://github.com/bitcoin/bitcoin/pull/26988) 1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851. 2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808. ACKs for top commit: 0xB10C: ACK 28bac81a346c0b68273fa73af924f7096cb3f41d willcl-ark: reACK 28bac81a346c0b68273fa73af924f7096cb3f41d achow101: ACK 28bac81a346c0b68273fa73af924f7096cb3f41d brunoerg: reACK 28bac81a346c0b68273fa73af924f7096cb3f41d theStack: Code-review ACK 28bac81a346c0b68273fa73af924f7096cb3f41d Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a