The descriptor wallets allow an application to track coins of multiple
descriptors in a single wallet. However, such an application would not
previously be able to (easily) tell what received coin "belongs" to what
descriptor.
This commit tackles this issues by adding a "wallet_desc" entry to the
entries for received coins in 'listsinceblock'.
fae5ce8795 univalue: Return more detailed type check error messages (MacroFake)
fafab147e7 move-only: Move UniValue::getInt definition to keep class with definitions only (MacroFake)
Pull request description:
Print the current type and the expected type
ACKs for top commit:
aureleoules:
ACK fae5ce8795.
Tree-SHA512: 4ae720a012ff8245baf5cd7f844f93b946c58feebe62de6dfd84ebc5c8afb988295a94de7c01aef98aaf4c6228f7184ed622f37079c738924617e0f336ac5b6e
d2ed97656b wallet: Precompute Txdata after setting PSBT inputs' UTXOs (Andrew Chow)
Pull request description:
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.
ACKs for top commit:
instagibbs:
reACK d2ed97656b
Sjors:
ACK d2ed97656b
aureleoules:
ACK d2ed97656b.
Tree-SHA512: 71beb6c7946096e82cfca83f36277302aa9e69d27b4f6d73d7d8f2f9f0ea1c0d653e846fa6aebee5e4763f56f950b4481240e953f6a2412caa84908d519171e1
1be7964189 test, wallet: Add mempool rescan test for import RPCs (Fabian Jahr)
833ce76df7 rpc, wallet: Document mempool rescan after importdescriptor, importwallet (Fabian Jahr)
0e396d1ba7 rpc, wallet: Document mempool scan after importmulti (Fabian Jahr)
e6d3ef8586 rpc, wallet: Document mempool scan after importpubkey (Fabian Jahr)
6d3db52e66 rpc, wallet: Document and test mempool scan after importprivkey (João Barbosa)
3abdbbb90a rpc, wallet: Document and test mempool scan after importaddress (João Barbosa)
236239bd40 wallet: Rescan mempool for transactions as well (Fabian Jahr)
Pull request description:
This PR picks up the work from #18964 and closes#18954.
It should incorporate all the unaddressed feedback from the PR:
- Mempool rescan now expanded to all relevant import* RPCs
- Added documentation in the help of each RPC
- More tests
ACKs for top commit:
Sjors:
re-utACK 1be7964189 (only a test change)
achow101:
ACK 1be7964189
w0xlt:
reACK 1be7964189
Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5
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
