cb3e9a1e3f Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa30676541 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8a LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b0 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e3756 mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741 test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`.
More context can be gleaned from the commit messages.
-----
One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future.
ACKs for top commit:
glozow:
re ACK cb3e9a1e3f via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f🔒
ryanofsky:
Code review ACK cb3e9a1e3f
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
ffc79b8e49 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756a Miniscript support in output descriptors (Antoine Poinsot)
4a082887be qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817 miniscript: don't check for top level validity at parsing time (Antoine Poinsot)
Pull request description:
This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.
A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at https://github.com/bitcoin-core/qa-assets/pull/92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.
This PR contains code and insights from Pieter Wuille.
ACKs for top commit:
Sjors:
re-utACK ffc79b8e49
achow101:
ACK ffc79b8e49
w0xlt:
reACK ffc79b8e49
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
613e221149 test: remove unnecessary parens (Suhas Daftuar)
e939cf2b76 Remove atomic for m_last_getheaders_timestamp (Suhas Daftuar)
Pull request description:
Eliminate the unnecessary atomic guarding `m_last_getheaders_timestamp`, which is only accessed in a single thread (thanks to MarcoFalke for pointing this out).
Also address a nit that came up in #25454.
ACKs for top commit:
MarcoFalke:
review ACK 613e221149
vasild:
ACK 613e221149
Tree-SHA512: 6d6c473735b450b8ad43aae5cf16ed419154d72f4a05c0a6ce6f26caecab9db2361041398b70bf9395611c107d50897f501fa5fdbabb2891144bbc2b479dfdad
2ef5294a5b rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5b
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
a9790ba95f [test] persist prioritisation of transactions not in mempool (glozow)
Pull request description:
We persist tx prioritisation/fee deltas in mempool.dat (see `DumpMempool`). It seems we have test coverage for persistence of modified fees of mempool entries (see `vinfo` loop), but not for the prioritisation of transactions not in mempool (see `mapDeltas`).
Relevant: https://github.com/bitcoin/bitcoin/pull/25487#discussion_r917490221
ACKs for top commit:
darosior:
utACK a9790ba95f
w0xlt:
ACK a9790ba95f
Tree-SHA512: 3f2769a917041f12414584f69b2239eef57586f9975869e826f356633fcaf893fde15500619b302e7663de14f3661c6cba22c7524cab5286e715e2c105726521
If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.
Also adds a test for this behavior.
0ee43d13e9 test: refactor rpc_signrawtransaction.py (Ayush Sharma)
Pull request description:
`rpc_signrawtransaction.py` currently tests the `signrawtransactionwithkey` and `signrawtransactionwithwallet` RPCs.
This PR splits `rpc_signrawtransaction.py` into
1. `rpc_signrawtransactionwithkey.py`: the tests for `signrawtransactionwithkey` are moved here and this test can now be run with the wallet disabled.
2. `wallet_signrawtransactionwithwallet.py`: wallet only tests for `signrawtransactionwithwallet.py`
ACKs for top commit:
aureleoules:
tACK 0ee43d13e9.
Tree-SHA512: c7bd2ea746345c978eae231a781fc52953b9d277eb9f8abb9c3270fe1d9f678f23f3784377d7303a2aa23d7ad90097245e517d386b27b4e0016585dfddcb9d49
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)
Pull request description:
### Context
The wallet's `m_address_book` field is being accessed directly from several places across the sources.
### Problem
Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.
### Solution
Encapsulate `m_address_book` access inside the wallet.
-------------------------------------------------------
Extra Note:
This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).
ACKs for top commit:
achow101:
ACK d69045e291
theStack:
ACK d69045e291✅
w0xlt:
ACK d69045e291
Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
4c9666bd73 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard)
aae66ab43d Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard)
3e27e31727 Introduce `mempoolfullrbf` node setting. (Antoine Riard)
Pull request description:
This is ready for review.
Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].
This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior.
I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread.
[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html
Follow-up suggestions :
- soft-enable opt-in RBF in the wallet : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1154918789
- p2p discovery and additional outbound connection to full-rbf peers : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1156044401
- match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698
ACKs for top commit:
instagibbs:
reACK 4c9666bd73
glozow:
ACK 4c9666bd73, a few nits which are non-blocking.
w0xlt:
ACK 4c9666bd73
Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
fad690ba0a test: Remove -acceptnonstdtxn=1 from feature_rbf.py (MacroFake)
fa5059b7df test: Make the scriptPubKey of MiniWallet created txs mutable (MacroFake)
fa29245827 test: Allow setting sequence per input in MiniWallet create_self_transfer_multi (MacroFake)
fac3800d2c test: Allow amount_per_output in MiniWallet create_self_transfer_multi (MacroFake)
2222842ae7 test: Allow absolute fee in MiniWallet create_self_transfer (MacroFake)
Pull request description:
On the main network, nonstandard transactions are hardly relayed, so it seems odd that one of our policy test requires a policy setting opposite of the norm.
Surely it is also important to test that nonstandard transactions can be replaced. However, rbf code should not care about the standardness at all. Moreover, I think testing nonstandardness rbf is of lower priority than testing the stuff that actually happens in production.
ACKs for top commit:
theStack:
re-ACK fad690ba0a
jamesob:
ACK fad690ba0a ([`jamesob/ackr/25522.1.MarcoFalke.test_remove_acceptnonstd`](https://github.com/jamesob/bitcoin/tree/ackr/25522.1.MarcoFalke.test_remove_acceptnonstd))
Tree-SHA512: e0a0c808bccdddf738fb6a84e5e5672d7c341bffd941c4f0c232112bfc68265fa81a2e42ddcab107d586358ffdb3dccc46bb5533d46999fd9ab024169dac0f78
eac1099e00 test: remove wallet dependency from mempool_updatefromblock.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests(`mempool_updatefromblock.py`) to be run with the wallet disabled.
ACKs for top commit:
aureleoules:
tACK eac1099e00.
Tree-SHA512: 9734815f2d2e65e8813bd776cf1d847a55ba4181e218c5e7b066ec69a556261069214f675681d672f5d7b0ba8e06342c4a456619dcc005cbf5618a0527303b7f
This new node policy setting enables to accept replaced-by-fee
transaction without inspection of the replaceability signaling
as described in BIP125 "explicit signaling".
If turns on, the node mempool accepts transaction replacement
as described in `policy/mempool-replacements.md`.
The default setting value is `false`, implying opt-in RBF
is enforced.
c0a5fceee9 test: Add test for erase orphan tx conflicted by block (Hennadii Stepanov)
fa45bb2119 test: Add test for erase orphan tx included by block (Hennadii Stepanov)
5c049780c8 test: Add test for erase orphan tx from peer (Hennadii Stepanov)
Pull request description:
This PR adds test coverage for the following cases:
- erase orphan transactions when a peer is disconnected
- erase an orphan transaction when it is included in a new tip block
- erase an orphan transaction when it is conflicted with other transactions included in a new tip block
Found useful while working on #19374.
ACKs for top commit:
aureleoules:
tACK c0a5fceee9 (`make check` and `test/functional/test_runner.py`).
kouloumos:
ACK c0a5fceee9 with a nit per https://github.com/bitcoin/bitcoin/pull/19393#discussion_r899156623.
pg156:
Reviewed to c0a5fceee9. Concept ACK. Agree due to the lack of RPC calls to inspect orphan pool, using `assert_debug_log` to match strings in log is a reasonable way to test.
Tree-SHA512: 98f8deeee2d1c588c7e28a82e513d4a18655084198369db33fe2710458251eeaffed030626940072d7576f57fcbf7d856d761990129e2ca9e372d2ccbd86d07d
99f4785cad Replace GetTime() with NodeClock in MaybeSendGetHeaders() (Suhas Daftuar)
abf5d16c24 Don't send getheaders message when another request is outstanding (Suhas Daftuar)
ffe87db247 Cleanup received_new_header calculation to use WITH_LOCK (Suhas Daftuar)
6d95cd3e74 Move peer state updates from headers message into separate function (Suhas Daftuar)
2b341db731 Move headers direct fetch to end of ProcessHeadersMessage (Suhas Daftuar)
29c4518522 Move headers-direct-fetch logic into own function (Suhas Daftuar)
bf8ea6df75 Move additional headers fetching to own function (Suhas Daftuar)
9492e93bf9 Add helper function for checking header continuity (Suhas Daftuar)
7f2450871b Move handling of unconnecting headers into own function (Suhas Daftuar)
Pull request description:
Change `getheaders` messages so that we wait up to 2 minutes for a response to a prior `getheaders` message before issuing a new one.
Also change the handling of the `getheaders` message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a `getheaders` during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's `getheaders` message, even if you have nothing to give. This also avoids a lot of functional tests breaking.
This PR also reworks the headers processing logic to make it more readable.
ACKs for top commit:
ajtowns:
ACK 99f4785cad ; code review, check over new logic of when to send getheaders messages
dergoegge:
Code review ACK 99f4785cad
mzumsande:
Code Review ACK 99f4785cad
sipa:
utACK 99f4785cad
w0xlt:
tACK 99f4785cad Good improvement in the code.
Tree-SHA512: b8a63f6f71ac83e292edc0200def7835ad8b06b2955dd34e3ea6fac85980fa6962efd31d689ef5ea121ff5477ec14aafa4bbe2d0db134c05f4a31a57a8ced365
By specifying the `dustrelayfee=0` option instead of the more
generic `acceptnonstdtxn=1`, we can be more specific about what
part of the transaction is non-standard and can be sure that all
other aspects follow the standard policy.
The USDT interface exposes process internals via the tracepoints. This
means, the USDT interface tests somewhat awardly depend on these
internals. If internals change, the tests have to adopt to that change.
Previously, the USDT interface tests weren't run in the CI so changes
could break the USDT interface tests without being noticed (e.g.
https://github.com/bitcoin/bitcoin/pull/25486).
In fa13375aa3 a 'self.rescan_utxos()' call
was added in the 'generate()' function of the test framework.
'rescan_utxos()' causes the UTXO cache to be flushed. In the USDT
interface tests for the 'utxocache:flush' trancepoint, 'generate()' is
used. As the utxo cache is now flushed more often, the number of flushes
the tests expectes need to be adopted. Also, the utxo cache has now a
different size when being flushed.
The utxocache tracepoint is tested by shutting the node down and
pruning blocks, to test the 'for_prune' argument.
Changes:
- A list 'expected_flushes' is now used which contains 'mode',
'for_prune', and 'size' for each expected flush.
- When a flush happens, the expected-flush is removed from the list.
This list is checked to be empty (unchanged).
- Previously, shutting down caused these two flushes:
UTXOCacheFlush(duration=*, mode=ALWAYS, size=104, memory=*, for_prune=False)
UTXOCacheFlush(duration=*, mode=ALWAYS, size=0, memory=*, for_prune=False)
now it causes these flushes:
UTXOCacheFlush(duration=*, mode=ALWAYS, size=2, memory=*, for_prune=False)
UTXOCacheFlush(duration=*, mode=ALWAYS, size=0, memory=*, for_prune=False)
The 104 UTXOs flushed previously were mainly coinbase UTXOs generated
in previous tests and the test setup. These are now already flushed.
- In the 'for_prune' test we previously hooked into the tracepoint
before mining blocks. This changed to only get notified about the
tracepoint being triggered for the prune. Here, the utxo cache is
empty already as it has just been flushed in 'generate()'.
old:
UTXOCacheFlush(duration=*, mode=NONE, size=350, memory=*, for_prune=True)
new:
UTXOCacheFlush(duration=*, mode=NONE, size=0, memory=*, for_prune=True)
This makes sure to NOT hook into other bitcoind binaries run in
paralell in the test framework. We only want to trace the intended
binary.
In interface_usdt_utxocache.py:
While testing the utxocache flush with pruning, bitcoind is
restarted and we need to hook into the new PID again.
50ba6697f3 remove unused functions (Ayush Sharma)
eec23dad1e test: remove wallet dependency from feature_nulldummy.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (`feature_nulldummy.py`) to be run even with the Bitcoin Core wallet disabled.
Commit 1: removes wallet dependency and `test_runner.py` is edited to make sure the test only runs once.
Commit 2: the functions `create_transaction()` and `create_raw_transaction()` in `blocktools.py` are no longer needed and hence removed.
ACKs for top commit:
kouloumos:
re-ACK 50ba6697f3, all comments have been addressed.
Tree-SHA512: 3bc3d2766e53dba3d56a03f2c476442608ac693f51d84f4632a22a2cf169bc02c10bf92b676f7d57acb4f0ad86f307d37ab63f936b44b3585ee3c9d08cd0335f
e866f0d066 [functional test] submitrawpackage RPC (glozow)
fa076515b0 [rpc] add new submitpackage RPC (glozow)
Pull request description:
It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist.
Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp.
ACKs for top commit:
t-bast:
Tested ACK against eclair e866f0d066
ariard:
Code Review ACK e866f0d0
instagibbs:
code review ACK e866f0d066
Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
475aae846e test: pass `datacarriersize` option for tests using large outputs (instead of `acceptnonstdtxn`) (Sebastian Falbesoner)
b1ba3ed155 test: let `gen_return_txouts` create a single large OP_RETURN output (Sebastian Falbesoner)
f319287d81 test: assert serialized txouts size of `gen_return_txouts` helper (Sebastian Falbesoner)
Pull request description:
By specifying the `datacarriersize` option instead of the more generic `acceptnonstdtxn` for functional tests, we can be more specific about what part of the transaction is non-standard and can be sure that all other aspects follow the standard policy. Transactions with more than one OP_RETURN output are [never considered standard](749b80b29e/src/policy/policy.cpp (L149-L153)), i.e. we have to change the `gen_return_txouts` helper to create only a single output in order to get rid of the `acceptnonstdxtn` option. Note that on master there is currently no test using the `datacarriersize` parameter, so this PR indirectly also increases the test coverage.
The change affects the tests `mempool_limit.py`, `mining_prioritisetransaction.py` (call `gen_return_txouts` directly) and `feature_maxuploadtarget.py` (calls `gen_return_txouts` indirectly via the `mine_large_block(...)` helper).
Top commit has no ACKs.
Tree-SHA512: c17f032e00d28f5e5880a4d378773fbc8b1995ea9c377f237598d412628fe117f497a44ebdfa8af8cd8a3b1e3127e0cf7692efbf5c833c713764a71a85301f23
rpc_signrawtransaction.py is split into rpc_signrawtransactionwithkey.py and wallet_signrawtransactionwithwallet.py.
rpc_signrawtransactionwithkey.py can be run with the wallet disabled.
By specifying the `datacarriersize` option instead of the more
generic `acceptnonstdtxn`, we can be more specific about what
part of the transaction is non-standard and can be sure that all
other aspects follow the standard policy.
Transactions with more than one datacarrier (OP_RETURN) output
are never considered standard, i.e. this change is necessary in
order to to get rid of the `acceptnonstdtxn` option for some
tests.
d1684beabe fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30 init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c412 mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b10301 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf35 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014 mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd3 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321d scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd81 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5e init: Only determine maxmempool once (Carl Dong)
386c9472c8 mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a6 scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca6 ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe🍜
ryanofsky:
Code review ACK d1684beabe. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
Change getheaders messages so that we wait up to 2 minutes for a response to a
prior getheaders message before issuing a new one.
Also change the handling of the getheaders message sent in response to a block
INV, so that we no longer use the hashstop variable (including the hash stop
will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a getheaders during IBD with an empty headers message
(rather than nothing) -- this better conforms to the intent of the new logic
that it's better to not ignore a peer's getheaders message, even if you have
nothing to give. This also avoids a lot of functional tests breaking.
p2p_segwit.py is modified to use this same strategy, as the test logic (of
expecting a getheaders after a block inv) would otherwise be broken.
Better to be explicit when it comes to time to avoid unintentional bugs.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MEMPOOL_EXPIRY" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_HOURS@g"
-END VERIFY SCRIPT-
b80de4c505 test: Test signing psbts without explicitly having scripts (Andrew Chow)
a73b56888a wallet: also search taproot pubkeys in FillPSBT (Andrew Chow)
6cff82722f sign: Use sigdata taproot spenddata when signing (Andrew Chow)
5f12fe3f36 psbt: Implement merge for Taproot fields (Andrew Chow)
1ece9a3715 psbt, test: Check for taproot fields in taproot psbt test (Andrew Chow)
496a1bbe5e taproot: Use pre-existing signatures if available (Andrew Chow)
0ad21e7c55 tests: Test taproot fields for PSBT (Andrew Chow)
103c6fd279 psbt: Remove non_witness_utxo for segwit v1+ (Andrew Chow)
7dccdd3157 Implement decodepsbt for Taproot fields (Andrew Chow)
ac7747585f Fill PSBT Taproot output data to/from SignatureData (Andrew Chow)
25b6ae46e7 Assert that TaprootBuilder is Finalized during GetSpendData (Andrew Chow)
3ae5b6af21 Store TaprootBuilder in SigningProviders instead of TaprootSpendData (Andrew Chow)
4d1223e512 Fetch key origins for Taproot keys (Andrew Chow)
52e3f2f88e Fill PSBT Taproot input data to/from SignatureData (Andrew Chow)
05e2cc9a30 Implement de/ser of PSBT's Taproot fields (Andrew Chow)
d557eff2ad Add serialization methods to XOnlyPubKey (Andrew Chow)
d43923c381 Add TaprootBuilder::GetTreeTuples (Andrew Chow)
ce911204e4 Move individual KeyOriginInfo de/ser to separate function (Andrew Chow)
Pull request description:
Implements the Taproot fields for PSBT described in [BIP 371](https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki).
ACKs for top commit:
laanwj:
Code review ACK b80de4c505
Tree-SHA512: 50b79bb44f353c9ec2ef4c98aac08a81eba560987e5264a5684caa370e9c4e7a8255c06747fc47749511be45b32d01492e015f92b82be8d22bc8bf192073bd26
fa07f84e31 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke)
fa52cf8e11 refactor: Replace feeDelta by m_modified_fee (MarcoFalke)
Pull request description:
Signed integer overflow is UB in theory, but not in practice. Still,
it would be nice to avoid this UB to allow Bitcoin Core to be
compiled with sanitizers such as `-ftrapv` or ubsan.
It is impossible to predict when and if an overflow occurs, since
the overflow caused by a prioritisetransaction RPC might only be
later hit when descendant txs are added to the mempool.
Since it is impossible to predict reliably, leave it up to the user
to use the RPC endpoint responsibly, considering their mempool
limits and usage patterns.
Fixes: #20626Fixes: #20383Fixes: #19278
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34146 / https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47132
## Steps to reproduce
Build the code without the changes in this pull.
Make sure to pass the sanitizer flag:
```
./autogen.sh && ./configure --with-sanitizers=signed-integer-overflow && make clean && make -j $(nproc)
```
### Reproduce on RPC
```
./src/bitcoind -chain=regtest -noprinttoconsole &
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 9123456789123456789 + 9123456789123456789 cannot be represented in type 'long int'
./src/bitcoin-cli -chain=regtest stop
```
### By fuzzing
```
wget https://github.com/bitcoin/bitcoin/files/8921302/clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
FUZZ=validation_load_mempool ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 7214801925397553184 + 2314885530818453536 cannot be represented in type 'long int'
|> validation_load_mempool: succeeded against 1 files in 0s.
ACKs for top commit:
vasild:
ACK fa07f84e31
dunxen:
ACK fa07f84
LarryRuane:
ACK fa07f84e31
Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df
fafee78188 rpc: Return incrementalrelayfee in getmempoolinfo (MacroFake)
Pull request description:
Seems odd to return other policy info, but not the incremental relay fee
ACKs for top commit:
1440000bytes:
ACK fafee78188
w0xlt:
Code Review ACK fafee78188
jarolrod:
tACK fafee78188
Tree-SHA512: faad0af6c039b8257acbeac913bc5dcdb2ea2db304c95e52601536c8de60eb1186e9fbb4a64a68adf476605f18022aeda16a5644a0d7912592b0977e4c029638
e3d8d72703 test: Remove unnecessary block mining from importdescriptors test (Fabian Jahr)
Pull request description:
This removes generation of 6 blocks and replaces is with a `sync_all` in the `importdescriptors` test.
The generated blocks themself don't seem to serve any purpose in the test. Instead they could make the test flaky (although I did not find open issues pointing to this happening in practice in the CI). Right before the blocks being generated a transaction is created (L454) and later in the test this tx is assumed to be still in the mempool. If the nodes were to sync their mempools before the blocks are generated, the test fails. It currently only seems to work because one node sends the tx while the other generates the blocks and the mempools are not synced fast enough.
The `sync_all` is still needed to let nodes catch up at that point. Otherwise races happen further below which the generate call seems to have prevented so far.
ACKs for top commit:
laanwj:
Code review ACK e3d8d72703
Tree-SHA512: 14f3dc2938d779d1ad43e09a7e046523fc3c92f41df012833f279a2e88e74c2fcab301fe4f3fcc038bd8460ea1360725a8d1eb5b59acd1039495bacb484fd790
ceec6808d3 test: `-whitebind` and `-bind` with `-listen=0` should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9122e95f0/src/init.cpp (L872-L875)
ACKs for top commit:
laanwj:
Code review ACK ceec6808d3
Tree-SHA512: 03068abe7199b1235f029871ab87a3dd4943738c592ad62d82cdcd3e0201e627624960bd3ea1fc6fc1e7da4b8e215ba3393d1cb8130e1108049f764e51dc75c0
Rather than abusing the member variables self._priv_key and
self._address to determine the MiniWallet mode, save it explicitly
instead in the constructor to increase the readability and
maintainability of the code.
dcf36fe8e3 test: implement 'bech32m' mode for `getnewdestination()` helper (Sebastian Falbesoner)
1999dcfa40 test: add helpers for creating P2TR scripts/addresses from output key (Sebastian Falbesoner)
Pull request description:
This PR adds the missing 'bech32m' mode for the `getnewdestination()` helper and sets it as default, i.e. the function returns a tuple (output x-only-pubkey, scriptPubKey, taproot address) now if not specified otherwise. In a preparation commit, the helpers `output_key_to_p2tr{_script}` are introduced. Note that in contrast to all other common script output types, there are usually _two_ keys involved in creating a taproot output (internal key and output key), hence the prefix `output_` is used to clarify that the output key is expected and the helpers don't do any key tweaking.
Thanks to michaelfolkson (for pointing out this TODO that I forgot about) and sipa (for patiently explaining basic things about BIP341).
ACKs for top commit:
michaelfolkson:
ACK dcf36fe8e3
w0xlt:
reACK dcf36fe8e3
Tree-SHA512: 5bb8d5fd96c63092ede10c3f022ffb2e13c14e333c4aa73348d95deb70cbf0a74745218dc4a7c419eb846793dd69e8217a7b4332a13ae2b2758e100b51fb1a9f
7832e9438f test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379f wallet: do not count wallet utxos as external (S3RK)
Pull request description:
Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.
Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.
ACKs for top commit:
achow101:
re-ACK 7832e9438f
Xekyo:
re-ACK 7832e9438f
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
d873ff96e5 refactor: cleanups post unsubtree'ing univalue (fanquake)
e2aa7047f9 refactor: un-subtree univalue (fanquake)
Pull request description:
At this point, maintaining Univalue as a subtree doesn’t serve much purpose, other than being an inconvenience for making changes to the code (along with polluting our repo with a number of files we don’t use). Our [Univalue fork](https://github.com/bitcoin-core/univalue-subtree) currently deviates from the [upstream API](https://github.com/jgarzik/univalue), and for some time has been marked as not-maintained for use by other projects (I'm not aware of any that use it). The upstream Univalue is not maintained, and has not been for some time. There are no new releases, bugs remain unfixed, and PR's we've upstreamed, https://github.com/jgarzik/univalue/pulls, are not being commented on/merged.
Another substantial benefit of no-longer maintaining a subtree is removing the rather awkward work-flow currently required to make changes to the Univalue code, particularly breaking changes / introducing new features, e.g. https://github.com/bitcoin-core/univalue-subtree/pull/27. We need to dance around and merge changes to our fork, with a flag, then pull them down here, then switch to using the new code, then go back to our Univalue repo, and remove the old code / flag, then pull the repo down here again, and remove our usage of the flag. Quite the overcomplicated mess.
With this PR I'm proposing we stop treating Univalue like a subtree, or upstream project/fork, and going forward, treat it as part of this codebase, which we can refactor directly (with pulls to this repo. Ideally, after this is merged, our univalue subtree repo could be marked as "archived". In this repo, I think there is a good chance that the Univalue code will ultimately be refactored away into "modern" C++, i.e using `std::variant` (at least one person has played around with doing this).
Univalue history:
- Subtree first introduced: https://github.com/bitcoin/bitcoin/pull/6637
- `--system-univalue` option introduced: https://github.com/bitcoin/bitcoin/pull/7349
Suggestion was to use system Univalue by default.
This was pushed back on by contributors, as well as the [upstream Univalue](https://github.com/jgarzik/univalue) maintainer (jgarzik).
- Our fork's README was updated to say `It is not maintained for usage by other projects. Notably, the API may break in non-backward-compatible ways.` : https://github.com/bitcoin-core/univalue-subtree/pull/17
- Our fork README additionally updated to say `the API is broken in non-backward-compatible ways.` : https://github.com/bitcoin-core/univalue-subtree/pull/30
- `--system-univalue` option removed: https://github.com/bitcoin/bitcoin/pull/22646
- Univalue "subtree" removed: This PR.
Guix Build (x86_64):
```bash
06748985a9a386457d10a411b5afe1d59536e5653ec9c5bc8ac8410cd715d073 guix-build-d873ff96e51a/output/aarch64-linux-gnu/SHA256SUMS.part
57d81891f6d4ae417dd3bcbfc90839600e103da9c7d7b09dbebb82f0119241f3 guix-build-d873ff96e51a/output/aarch64-linux-gnu/bitcoin-d873ff96e51a-aarch64-linux-gnu-debug.tar.gz
7bb70d3b67253f5e8e5af8158bbf1b4b3e25e782f951d3defb7976534ae67d62 guix-build-d873ff96e51a/output/aarch64-linux-gnu/bitcoin-d873ff96e51a-aarch64-linux-gnu.tar.gz
b1acb90877d6e3b8d4bd2d57103889e0474263e4153f302eba8cb304fd1aecd7 guix-build-d873ff96e51a/output/arm-linux-gnueabihf/SHA256SUMS.part
91f9f65aebc131522cae5b523359c62e402a2c929670e1cca19d6a2760d29e04 guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf-debug.tar.gz
1fc3ed39bfc95592503b8dd11f468240deca4fb757f9adb08a0f07f5c0690837 guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf.tar.gz
a5cf5bd0ee0de92fb03f6bca91cfa6667ed77885112e71dd92a82bbd8670141e guix-build-d873ff96e51a/output/arm64-apple-darwin/SHA256SUMS.part
f6715399cebb5ac0a09f190fe805146c13d1e8eba57401541d0628da3badc588 guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.dmg
07cf82cab4e459ed4e862fc3a2903e49ac750adc6b6fe0534ec165f00e666230 guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.tar.gz
81bc076aa415183109e2848fa3cc0265b34f6af3e75b76bcbc6cff524db76a0f guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin.tar.gz
8751b05a3395d668e31217c92cbce9c131aa3566b3784a7e3544adf34fc89fe8 guix-build-d873ff96e51a/output/dist-archive/bitcoin-d873ff96e51a.tar.gz
526b7780a16a3de3c6006606d3d7a8c2ca565ef28669e2f6f303349a252e4977 guix-build-d873ff96e51a/output/powerpc64-linux-gnu/SHA256SUMS.part
ff917a50d2b20d41a5954e1ba1e8fb39498a9c8867828483af3f501573148ede guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu-debug.tar.gz
0311455c821ad392013fc3999a2b2d027fdb5c28e7eb6c3fea9cec29f3730d2d guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu.tar.gz
983c2553990eb7cebb26e1a0a3e5a9308259dea60d0b64ab6782892d02a7abc1 guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/SHA256SUMS.part
aba604827d969348671ec3f36dbf37469292715d3f756a7f44a0a5243dbe02f3 guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu-debug.tar.gz
e450bd82020d5086f3bb0a23181263315cc05eaf6e5809d0a2115bff4e7ddb2e guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu.tar.gz
476e8e2c80498b241af154abd9112bd2767110c0d6d7e9fa11761de716cb760f guix-build-d873ff96e51a/output/riscv64-linux-gnu/SHA256SUMS.part
a76435b3492efcd9af47ad652170605fad50691fd5aff2b46bce0bd08014879e guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu-debug.tar.gz
83985d409cd90bf7120cf7902ee442595d28a1469b7c600b666ef901981e5190 guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu.tar.gz
61c89850244ddf5813ff80c242eff89925d30bccadfa5cb63e968c3af49eb964 guix-build-d873ff96e51a/output/x86_64-apple-darwin/SHA256SUMS.part
cd219fab8918b061a342357d298aca0c044feb34c6d50a7851d5d3bf18cec267 guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.dmg
1170d3fdb199fbfca2c20b2a77cc81a6fe24b7e4973543a4461e887f14ac68e9 guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.tar.gz
71e93297ed8c581a7ed32a6948ef7b1ea2e7c43cb054181de3b5f604f7a2c28b guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin.tar.gz
fc8b7b670de9d175775e73df47dc855581c873a9be4adf1d81a4dbb2831d5348 guix-build-d873ff96e51a/output/x86_64-linux-gnu/SHA256SUMS.part
5703b02c2647f9997aa5ca12514d6a54b1eb2e29046223ca062383326b95894f guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu-debug.tar.gz
bab4b932b83476cf6fc2e0b5bf0d2203287f7fd0d1a968e325f2edd5b1d8415b guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu.tar.gz
5d180b0415fa8e825d46928c168cb1ae6e27016841b2ff8e190bf13879a5545c guix-build-d873ff96e51a/output/x86_64-w64-mingw32/SHA256SUMS.part
d469695a32f6414b25fef7b5fdfda4d854071450ba25148a1dce468114fa9057 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-debug.zip
2e7d4e533a5998863c115c586c61b75b4039cd329e12ed24cff78b7f16b6ea57 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-setup-unsigned.exe
3dabbd627b532beef57c3d4b5bd30c93c5ea74c492918484cf24685aca8d7bc4 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-unsigned.tar.gz
3a40660fba08f7632efd1f73c198f8298db33eab6ef5eaca88b997d95fc31f29 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64.zip
```
Guix Build (arm64):
```bash
0e764679199358fc321dcfcb58c6302e6518f55b3fd27bdd47f2da2a826ba16a guix-build-d873ff96e51a/output/arm-linux-gnueabihf/SHA256SUMS.part
5955d28e6d56e5a3297dab723b8478f1b0bb7f5b86476c581339122f34cc7f14 guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf-debug.tar.gz
49c68bc0066f709be68f1e5731425d51fb3cb8062a24aa9fa599987165759cad guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf.tar.gz
ca678d4eb27c9fa3c527211c0ccb145322a15f327545b5c82f1d1b8d3c310e5a guix-build-d873ff96e51a/output/arm64-apple-darwin/SHA256SUMS.part
38366d7fbd769b426f1097e966abe39f01a7ce743f6af1cd0f228b1801d3c87f guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.dmg
0c05dc9c17f5d8237b3e003c2e4c715455c3868bd4cd014e2a15ceb152b27b9c guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.tar.gz
32676e1f9f07f3f77143f8b6038c943da6ba93b081232ec52c2ff940f9f7cc88 guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin.tar.gz
8751b05a3395d668e31217c92cbce9c131aa3566b3784a7e3544adf34fc89fe8 guix-build-d873ff96e51a/output/dist-archive/bitcoin-d873ff96e51a.tar.gz
bdae66515060cab0b362784f0b2019b77da0435f1732d3c91fabcfb5e8c675f6 guix-build-d873ff96e51a/output/powerpc64-linux-gnu/SHA256SUMS.part
8d837391310b4cdec2296a6e78a9f9b3ea2b3da7870881a5cedf86a3429c08c6 guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu-debug.tar.gz
efe825d6f36338bd4c0b427901b72d666f819858fb241a4211f03bbb738f6961 guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu.tar.gz
7494cf8c5f384ca3205b3ed44dd4c0edebcb9e0a6bf9c8e649fc6d99cc5a10b2 guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/SHA256SUMS.part
8ceeb21d7fce9e164dbb47b35d0551b59819075fc44dcea39603132340f80c41 guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu-debug.tar.gz
bfbbb20dc4e7b30444a52f5f57b5789b5d1edee80abdc8066129b48c59ee65c9 guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu.tar.gz
65d578b81b00a1032039362dc6be1a71368f390188e0f948829afd03b8858ed2 guix-build-d873ff96e51a/output/riscv64-linux-gnu/SHA256SUMS.part
e5233d7e7a8832893ff414c78eb3d4bca3ae30d1a1f789a23419c6739b203022 guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu-debug.tar.gz
fb6d9f5a063dc7752fcc2acc95a0052322d7c8c86d2c6373e0ceb949dcf22f49 guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu.tar.gz
61c89850244ddf5813ff80c242eff89925d30bccadfa5cb63e968c3af49eb964 guix-build-d873ff96e51a/output/x86_64-apple-darwin/SHA256SUMS.part
cd219fab8918b061a342357d298aca0c044feb34c6d50a7851d5d3bf18cec267 guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.dmg
1170d3fdb199fbfca2c20b2a77cc81a6fe24b7e4973543a4461e887f14ac68e9 guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.tar.gz
71e93297ed8c581a7ed32a6948ef7b1ea2e7c43cb054181de3b5f604f7a2c28b guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin.tar.gz
46e9b067ec385ee14642aebc5ec09d7d2382e0204eeb17dc64587013eddd5dff guix-build-d873ff96e51a/output/x86_64-linux-gnu/SHA256SUMS.part
23278b19daac51e7df65b817b79fc93562d0f4eb193ef87472456f4bed1464d7 guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu-debug.tar.gz
4d5e5e23f089a59185f62faf367d8ca86476e406e6b7bbc9e8950cd89d94534d guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu.tar.gz
eec8ab97ee9aceef8cb4e7cb5026225ffc5c7b8e8a6d376e8348020000e5af88 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/SHA256SUMS.part
a31819e67c373f30eafce8dbcb3d6d0c61d1dcf59c51023aa79321934f8a7d2a guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-debug.zip
2e7d4e533a5998863c115c586c61b75b4039cd329e12ed24cff78b7f16b6ea57 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-setup-unsigned.exe
3dabbd627b532beef57c3d4b5bd30c93c5ea74c492918484cf24685aca8d7bc4 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-unsigned.tar.gz
ec438531b4694913dbbf7c91920dcbd957354b164f807867c16a001898edf669 guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64.zip
```
ACKs for top commit:
laanwj:
Code review ACK d873ff96e5
MarcoFalke:
re-ACK d873ff96e5 only changes: 📼
Tree-SHA512: fc7d781e8cc0fc0a0080eb4b5019e91c55275e087149ed3b5abc6b691170b0ab76f1dd3ce9bb8846eef023897a89123e14751ce8facf2a170829858199904bff
e3609cdc01 doc: Update importaddress mention incompatibility with descriptor wallet (BrokenProgrammer)
Pull request description:
This is related to #25363 and offers a small update to the error messages from `EnsureLegacyScriptPubKeyMan` and `EnsureConstLegacyScriptPubKeyMan` to mention that they only are compatible with legacy wallets.
The RPC documentation for `importaddress` is also updated to mention this as well as guide the user to the alternative `importdescriptors` for cases when using descriptor wallets.
I'm thinking that we can introduce a "porting guide" document mentioned in #25363 in a separate PR since I would have to make myself more familiar with the subject before being able to tackle that.
ACKs for top commit:
laanwj:
Code review ACK e3609cdc01
achow101:
ACK e3609cdc01
Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
42b2fdfd5f test: remove unused `create_confirmed_utxos` helper (Sebastian Falbesoner)
Pull request description:
After more and more non-wallet tests have been converted to use MiniWallet (#25087, #24839, #24749 etc.), the `create_confirmed_utxos` helper is now not used anymore and can be removed. An alternative would be to create a MiniWallet version of `create_confirmed_utxos`, but it seems that it's not worth it, considering that would be only two lines (calling MiniWallet's `send_self_transfer_multi` with a subsequent `generate` call), see comment https://github.com/bitcoin/bitcoin/pull/24839#discussion_r896472729.
ACKs for top commit:
MarcoFalke:
cr ACK 42b2fdfd5f
Tree-SHA512: 274418156265a6071940f53cbcd77f6779af5e951cfa1e5efbf07a5c61487b521ee19f36b4105e5c0a808139d121e5e262e77525ea3d1486a0421f01abcf58fd
5a8c321444 test: check for `getblocktxn` request with out-of-bounds tx index (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `getblocktxn` message handler, in the case that any of the contained indices is out-of-bounds:
a05876619a/src/net_processing.cpp (L2180-L2183)
ACKs for top commit:
dunxen:
ACK 5a8c321
Tree-SHA512: 2743c2c6d8aed57b22f825aefd60ba3e670321b60625a42ea7248e7b0fc41c73e9a5945153567c02824ba3b5f0fce7f4125bffc974973fc608b6ffbe49e14b65
fafddafc2c refactor: Introduce PeerManagerImpl::RejectIncomingTxs (MacroFake)
Pull request description:
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167
ACKs for top commit:
MarcoFalke:
Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa2b5fe0c1 fafddafc2c`.
jnewbery:
Code review ACK fafddafc2c
mzumsande:
ACK fafddafc2c
Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490
Confirmed UTXOs in functional tests can simply be created by using
MiniWallet's `send_self_transfer_multi` method with a subsequent
`generate` call to mine a block.
ecff20db28 logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759f logging: add LogPrintfCategory to log unconditionally with category (Jon Atack)
Pull request description:
These are the next two commits from #25203.
- Add `LogPrintfCategory` to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the `lint-logs.py` and `lint-format-strings.py` scripts.
- Replace the log messages that manually print a category, with `LogPrintfCategory`. In upcoming commits, it will likely be used in many other cases, such as to replace `LogPrintf` where it makes sense.
ACKs for top commit:
klementtan:
Code Review ACK ecff20db28
laanwj:
Code review ACK ecff20db28
brunoerg:
ACK ecff20db28
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`,
so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented
differently. For example, it seems a bit confusing to disconnect
`block-relay-only` peers with `relay` permission when they send a tx
message, but not when they send an inv message. Also, keeping track of
their inv announcements seems both wasteful and confusing, as it does
nothing. This isn't possible in practice, as outbound connections do
not have permissions assigned, but sees fragile to rely on. Especially
in light of proposed changes to make that possible:
https://github.com/bitcoin/bitcoin/pull/17167
b167e536d0 test: refactor: use `create_lots_of_big_transactions` to dedup where possible (Sebastian Falbesoner)
8973eeb412 test: use MiniWallet for mining_prioritisetransaction.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_prioritisetransaction.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in #20078. Note that the adapted helper function `create_lots_of_big_transactions` is currently only used in this test, i.e. there was no need to change any others.
ACKs for top commit:
ayush933:
tACK b167e53
danielabrozzoni:
tACK b167e536d0
kouloumos:
ACK b167e536d0
furszy:
ACK b167e536
Tree-SHA512: ccae20d7d414a720efdeea9c2ae399aa53a3a0e7db72bff8d0cb75d90621a7ae7c019ba68d24f9d06f7b111f87ff33bb9d8e5aa08b763e606cf10268780e205c
