By throwing a custom exception from `Univalue::checkType` (instead of a plain
std::runtime_error) and catching it on the RPC server request handler.
So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
not the general RPC_MISC_ERROR (-1).
and also fix spelling in test/lint/lint-locale-dependence.py not caught by the
spelling linter and fix up a paragraph we are touching here in test/README.md.
The `sendall` RPC doesn't use `CreateTransactionInternal`as the rest of
the wallet RPCs and it never checks against the tx-size mempool limit.
Add a check for tx-size as well as test coverage for that case.
6f8e3818af sendall: check if the maxtxfee has been exceeded (ishaanam)
Pull request description:
Previously the `sendall` RPC didn't check whether the fees of the transaction it creates exceed the set `maxtxfee`. This PR adds this check to `sendall` and a test case for it.
ACKs for top commit:
achow101:
ACK 6f8e3818af
Xekyo:
ACK 6f8e3818af
glozow:
Concept ACK 6f8e3818af. The high feerate is unlikely but sendall should respect the existing wallet options.
Tree-SHA512: 6ef0961937091293d49be16f17e4451cff3159d901c0c7c6e508883999dfe0c20ed4d7126bf74bfea8150d4c1eef961a45f0c28ef64562e6cb817fede2319f1a
b00fc44ca5 test: add coverage for 'add_inputs' dynamic default value (furszy)
ddbcfdf3d0 RPC: bugfix, 'add_inputs' default value is true unless 'inputs' are provided (furszy)
Pull request description:
This bugfix was meant to be in #25685, but decoupled it to try to make it part of 24.0 release.
It's a truly misleading functionality.
This PR doesn't change behavior in any way. Just fixes two invalid RPC help messages and adds test
coverage for the current behavior.
#### Description
In both RPC commands `send()` and `walletcreatefundedpsbt` the help message says
that `add_inputs` default value is false when it's actually dynamically set by the following statement:
```c++
coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;
```
Which means that, by default, `add_inputs` is true unless there is any pre-set input, in which
case, the default is false.
ACKs for top commit:
achow101:
ACK b00fc44ca5
S3RK:
ACK b00fc44ca5
Tree-SHA512: 5c68a40d81c994e0ab6de0817db69c4d3dea3a9a64a60362531bf583b7a4c37d524b740905a3f3a89cdbf221913ff5b504746625adb8622788aea93a35bbcd40
Covered cases for send() and walletcreatefundedpsbt() RPC commands:
1. Default add_inputs value with no preset inputs (add_inputs=true):
Expect: automatically add coins from the wallet to the tx.
2. Default add_inputs value with preset inputs (add_inputs=false):
Expect: disallow automatic coin selection.
3. Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount).
Expect: include inputs from the wallet.
4. Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount).
Expect: only preset inputs are used.
5. Explicit add_inputs=true, no preset inputs (same as (1) but with an explicit set):
Expect: include inputs from the wallet.
Add functionality for activating a snapshot-based chainstate if one is
detected on-disk.
Also cautiously initialize chainstate cache usages so that we don't
somehow blow past our cache allowances during initialization, then
rebalance at the end of init.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This changes the snapshot's leveldb chainstate dir name from
`chainstate_[blockhash]` to `chainstate_snapshot`. This simplifies
later logic that loads snapshot data, and enforces the limitation
of a single snapshot at any given time.
Since we still need to persis the blockhash of the base block, we
write that out to a file (`chainstate_snapshot/base_blockhash`) for
later use during initialization, so that we can reinitialize the
snapshot chainstate.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
00eeb31c76 scripted-diff: rename CChainState -> Chainstate (James O'Beirne)
Pull request description:
Alright alright alright, I know: we hate refactors. We especially hate cosmetic refactors.
Nobody knows better than I that changing broad swaths of code out from under our already-abused collaborators, only to send a cascade of rebase bankruptcies, is annoying at best and sadistic at worst. And for a rename! The indignation!
But just for a second, imagine yourself. Programming `bitcoin/bitcoin`, on a sandy beach beneath a lapis lazuli sky. You go to type the name of what is probably the most commonly used data structure in the codebase, and you *only hit shift once*.
What could you do in such a world? You could do anything. [The only limit is yourself.](https://zombo.com/)
---
So maybe you like the idea of this patch but really don't want to deal with rebasing. You're in luck!
Here're the commands that will bail you out of rebase bankruptcy:
```sh
git rebase -i $(git merge-base HEAD master) \
-x 'sed -i "s/CChainState/Chainstate/g" $(git ls-files | grep -E ".*\.(py|cpp|h)$") && git commit --amend --no-edit'
# <commit changed?>
git add -u && git rebase --continue
```
---
~~Anyway I'm not sure how serious I am about this, but I figured it was worth proposing.~~ I have decided I am very serious about this.
Maybe we can have nice things every once in a while?
ACKs for top commit:
MarcoFalke:
cr ACK 00eeb31c76
hebasto:
ACK 00eeb31c76
glozow:
ACK 00eeb31c76, thanks for being the one to propose this
w0xlt:
ACK 00eeb31c76
Tree-SHA512: b828a99780614a9b74f7a9c347ce0687de6f8d75232840f5ffc26e02bbb25a3b1f5f9deabbe44f82ada01459586ee8452a3ee2da05d1b3c48558c8df6f49e1b1
2d0b4e4ff6 init: allow startup with -onlynet=onion -listenonion=1 (Vasil Dimov)
Pull request description:
It does not make sense to specify `-onlynet=onion` without providing a
Tor proxy (even if other `-onlynet=...` are given). This is checked
during startup. However, it was forgotten that a Tor proxy can also be
retrieved from "Tor control" to which we connect if `-listenonion=1`.
So, the full Tor proxy retrieval logic is:
1. get it from `-onion`
2. get it from `-proxy`
3. if `-listenonion=1`, then connect to "Tor control" and get the proxy
from there (was forgotten before this change)
Fixes https://github.com/bitcoin/bitcoin/issues/24980
ACKs for top commit:
mzumsande:
Tested ACK 2d0b4e4ff6
MarcoFalke:
ACK 2d0b4e4ff6 🕸
Tree-SHA512: d1d18e07a8a40a47b7f00c31cb291a3d3a9b24eeb28c5e4720d5df4997f488583a3a010d46902b4b600d2ed1136a368e1051c133847ae165e0748b8167603dc3
We were throwing two different errors for the same problematic:
* "Expected type {expected], got {type}" --> RPCTypeCheckArgument()
* "JSON value of type {type} is not of expected type {expected}" --> UniValue::checkType()
4f67336f11 test: verify best blockhash after invalidating an unknown block (brunoerg)
Pull request description:
Fixes#26051
Verify the best blockhash is the same after invalidating an unknown block, not the whole `getchaintip` response.
ACKs for top commit:
instagibbs:
ACK 4f67336f11
Tree-SHA512: 2d71743c1d3a317ef7b750f88437df71d1aed2728d9edac8b763a343406e168b97865ab25ec4c89caf09d002e076458376618cbd0845496375f7179633c88af9
2186608172 test: apply fixed feerate to avoid variable dynamic fees (stickies-v)
Pull request description:
Without specifying a feerate, we let the wallet decide on an appropriate feerate, which can be influenced by various factors
such as what's in the mempool. Since wallet_groups.py fails when feerates are unstable, we should use a fixed feerate across all nodes. The assumed feerate was 20 sats/vbyte, so this PR adopts that.
Closes#25940. I'm not 100% sure, but I think the increased tx relay speed introduced by #25865 caused the transactions to more quickly and often enter the other nodes' mempools, affecting their feerate calculation done in [`wallet:GetMinimumFeeRate()`](ea67232cdb/src/wallet/fees.cpp (L68-L72)) and thus deviating slightly from the expected 20 sats/vbyte.
Ran `wallet_groups.py` over 400 times without failure.
ACKs for top commit:
aureleoules:
ACK 2186608172.
glozow:
Approach ACK 2186608172
Tree-SHA512: 0ea467a67747e6f27369ccd0adacfb21cc36ef0ae728fb28b8ea18e409aab5bd3ede559d6cebb82da0b9703c0c8b2709d686feb3ae009ddf525aa253f44d5816
4b1d5a1053 test: invalidating an unknown block throws an error (brunoerg)
Pull request description:
While playing with `invalidateblock`, I unintentionally tried to invalidate an unknown block and it threw an error. Looking at the tests I just realized there is no test coverage for this case. This PR adds it.
Top commit has no ACKs.
Tree-SHA512: 25286ead809b3ad022e759127ef3134b271fbe76cb7b50ec2b0c7e2409da8d1b01dc5e80afe73e4564cc9c9c03487a1fe772aea3456988552d2f9c8fb34c730b
385f5a4c3f p2p: Don't query DNS seeds when both IPv4 and IPv6 are unreachable (Martin Zumsande)
91f0a7fbb7 p2p: add only reachable addresses to addrman (Martin Zumsande)
Pull request description:
Currently, `-onlynet` does not work well in connection with initial peer discovery, because DNS seeds only resolve to IPv6 and IPv4 adresses:
With `-onlynet=i2p`, we would load clearnet addresses from DNS seeds into addrman, be content our addrman isn't empty so we don't try to query hardcoded seeds (although these exist for i2p!), and never attempt to make an automatic outbound connection.
With `-onlynet=onion` and `-proxy` set, we wouldn't load addresses via DNS, but will make AddrFetch connections (through a tor exit node) to a random clearnet peer the DNS seed resolves to (see https://github.com/bitcoin/bitcoin/issues/6808#issuecomment-147652505), thus breaching the `-onlynet` preference of the user - this has been reported in the two issues listed below.
This PR proposes two changes:
1.) Don't load addresses that are unreachable (so that we wouldn't connect to them) into addrman. This is already the case for addresses received via p2p addr messages, this PR implements the same for addresses received from DNS seeds and fixed seeds. This means that in the case of `-onlynet=onion`, we wouldn't load fixed seed IPv4 addresses into addrman, only the onion ones.
2.) Skip trying the DNS seeds if neither IPv4 nor IPv6 are reachable and move directly to adding the hardcoded seeds from networks we can connect to. This is done by soft-setting `-dnsseed` to 0 in this case, unless `-dnsseed=1` was explicitly specified, in which case we abort with an `InitError`.
Fixes#6808Fixes#12344
ACKs for top commit:
naumenkogs:
utACK 385f5a4c3f
vasild:
ACK 385f5a4c3f
Tree-SHA512: 33a8c29faccb2d9b937b017dba4ef72c10e05e458ccf258f1aed3893bcc37c2e984ec8de998d2ecfa54282abbf44a132e97d98bbcc24a0dcf1871566016a9b91
This happens, for example, if the user specified -onlynet=onion or
-onlynet=i2p. DNS seeds only resolve to IPv4 / IPv6 addresses,
making their answers useless to us, since we don't want to make
connections to these.
If, within the DNS seed thread, we'd instead do fallback AddrFetch
connections to one of the clearnet addresses the DNS seed resolves to,
we might get usable addresses from other networks
if lucky, but would be violating our -onlynet user preference
in doing so.
Therefore, in this case it is better to rely on fixed seeds for networks we
want to connect to.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
It does not make sense to specify `-onlynet=onion` without providing a
Tor proxy (even if other `-onlynet=...` are given). This is checked
during startup. However, it was forgotten that a Tor proxy can also be
retrieved from "Tor control" to which we connect if `-listenonion=1`.
So, the full Tor proxy retrieval logic is:
1. get it from `-onion`
2. get it from `-proxy`
3. if `-listenonion=1`, then connect to "Tor control" and get the proxy
from there (was forgotten before this change)
Fixes https://github.com/bitcoin/bitcoin/issues/24980
3405f3eed5 test: Test that an unconfirmed not-in-mempool chain is rebroadcast (Andrew Chow)
10d91c5abe wallet: Deduplicate Resend and ReacceptWalletTransactions (Andrew Chow)
Pull request description:
Currently `ResendWalletTransactions` (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in `mapWallet`. This ends up being random as `mapWallet` is a `std::unordered_map`. However `ReacceptWalletTransactions` (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that `ResendWalletTranactions` will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining `ReacceptWalletTransactions` and `ResendWalletTransactions` into a new `ResubmitWalletTransactions` so that the iteration code and basic checks are shared.
A test has also been added that checks that such transaction chains are rebroadcast correctly.
ACKs for top commit:
naumenkogs:
utACK 3405f3eed5
1440000bytes:
reACK 3405f3eed5
furszy:
Late code review ACK 3405f3ee
stickies-v:
ACK 3405f3eed5
Tree-SHA512: 1240d9690ecc2ae8d476286b79e2386f537a90c41dd2b8b8a5a9c2a917aa3af85d6aee019fbbb05e772985a2b197e2788305586d9d5dac78ccba1ee5aa31d77a
f663b43df0 QA: rpc_blockchain: Test output of getblock verbosity 0, False, and True (Luke Dashjr)
Pull request description:
Currently getblock's "verbosity" is documented as a NUM, though it has a fallback to Boolean for the (deprecated?) "verbose" alias.
Since we've been doing more generic type-checking on RPC stuff, I think it would be a good idea to actually test the Boolean values work.
I didn't see an existing test for verbosity=0, so this adds that too.
ACKs for top commit:
aureleoules:
ACK f663b43df0.
Tree-SHA512: 321a7795a2f32e469d28879dd323c85cb6b221828030e2a33ad9afd35a648191151a79b04e359b2f58314e43360f81c25f05be07deb42f61efdf556850a7266c
88e7807e77 test: fix non-determinism in p2p_headers_sync_with_minchainwork.py (Suhas Daftuar)
Pull request description:
The test for node3's chaintips (added in PR25960) needs some sort of synchronization in order to be reliable.
ACKs for top commit:
mzumsande:
Code Review ACK 88e7807e77
satsie:
ACK 88e7807e77
Tree-SHA512: 5607c5b1a95d91e7cf81b695eb356b782cbb303bcc7fd9044e1058c0c0625c5f9e5fe4f4dde9d2bffa27a80d83fc060336720f7becbba505ccfb8a04fcc81705
Without specifying a feerate, we let the wallet decide on an
appropriate feerate, which can be influenced by various factors
such as what's in the mempool. Since wallet_groups.py fails when
feerates are unstable, we should use a fixed feerate across all nodes.
Closes#25940
9580480570 Update debug logging section in the developer notes (Jon Atack)
1abaa31aa3 Update -debug and -debugexclude help docs for severity level logging (Jon Atack)
45f9282162 Create BCLog::Level::Trace log severity level (Jon Atack)
2a8712db4f Unit test coverage for -loglevel configuration option (klementtan)
eb7bee5f84 Create -loglevel configuration option (klementtan)
98a1f9c687 Unit test coverage for log severity levels (klementtan)
9c7507bf76 Create BCLog::Logger::LogLevelsString() helper function (klementtan)
8fe3457dbb Update LogAcceptCategory() and unit tests with log severity levels (klementtan)
c2797cfc60 Add BCLog::Logger::SetLogLevel()/SetCategoryLogLevel() for string inputs (klementtan)
f6c0cc0350 Add BCLog::Logger::m_category_log_levels data member and getter/setter (Jon Atack)
2978b387bf Add BCLog::Logger::m_log_level data member and getter/setter (Jon Atack)
f1379aeca9 Simplify BCLog::Level enum class and LogLevelToStr() function (Jon Atack)
Pull request description:
This is an updated version of https://github.com/bitcoin/bitcoin/pull/25287 and the next steps in parent PR #25203 implementing, with Klement Tan, user-configurable, per-category severity log levels based on an idea by John Newbery and refined in GitHub discussions by Wladimir Van der Laan and Marco Falke.
- simplify the `BCLog::Level` enum class and the `LogLevelToStr()` function and add documentation
- update the logging logic to filter logs by log level both globally and per-category
- add a hidden `-loglevel` help-debug config option to allow testing setting the global or per-category severity level on startup for logging categories enabled with the `-debug` configuration option or the logging RPC (Klement Tan)
- add a `trace` log severity level selectable by the user; the plan is for the current debug messages to become trace, LogPrint ones to become debug, and LogPrintf ones to become info, warning, or error
```
$ ./src/bitcoind -help-debug | grep -A10 loglevel
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC:
info, debug, trace (default=info); warning and error levels are
always logged. If <category>:<level> is supplied, the setting
will override the global one and may be specified multiple times
to set multiple category-specific levels. <category> can be:
addrman, bench, blockstorage, cmpctblock, coindb, estimatefee,
http, i2p, ipc, leveldb, libevent, lock, mempool, mempoolrej,
net, proxy, prune, qt, rand, reindex, rpc, selectcoins, tor,
util, validation, walletdb, zmq.
```
See the individual commit messages for details.
ACKs for top commit:
jonatack:
One final push per `git range-diff a5d5569 ce3c4c9 9580480` (should be trivial to re-ACK) to ensure this pull changes no default behavior in any way for users or the tests/CI in order to be completely v24 compatible, to update the unit test setup in general, and to update the debug logging section in the developer notes.
klementtan:
reACK 9580480570
1440000bytes:
reACK 9580480570
vasild:
ACK 9580480570
dunxen:
reACK 9580480
brunoerg:
reACK 9580480570
Tree-SHA512: 476a638e0581f40b5d058a9992691722e8b546471ec85e07cbc990798d1197fbffbd02e1b3d081b4978404e07a428378cdc8e159c0004b81f58be7fb01b7cba0
53e7ed075c doc: Release notes and other docs for migration (Andrew Chow)
9c44bfe244 Test migratewallet (Andrew Chow)
0b26e7cdf2 descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow)
31764c3f87 Add migratewallet RPC (Andrew Chow)
0bf7b38bff Implement MigrateLegacyToDescriptor (Andrew Chow)
e7b16f925a Implement MigrateToSQLite (Andrew Chow)
5b62f095e7 wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow)
22401f17e0 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow)
35f428fae6 Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow)
ea1ab390e4 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow)
e664af2976 Apply label to all scriptPubKeys of imported combo() (Andrew Chow)
Pull request description:
This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s.
For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor.
There are also tests.
ACKs for top commit:
Sjors:
tACK 53e7ed075c
w0xlt:
reACK 53e7ed075c
Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
fae5bd9200 test: Fix wallet_balance intermittent issue (MacroFake)
Pull request description:
Diff to reproduce:
```diff
index d2ed97ca76..25cc2d5734 100755
--- a/test/functional/wallet_balance.py
+++ b/test/functional/wallet_balance.py
@@ -265,7 +265,7 @@ class WalletTest(BitcoinTestFramework):
self.nodes[0].invalidateblock(block_reorg)
self.nodes[1].invalidateblock(block_reorg)
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
- self.generatetoaddress(self.nodes[0], 1, ADDRESS_WATCHONLY, sync_fun=self.no_op)
+ self.generatetoaddress(self.nodes[0], 1, ADDRESS_WATCHONLY)
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
# Now confirm tx_orig
```
Example in CI:
```
test 2022-08-24T10:09:22.486000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_balance.py", line 269, in run_test
assert_equal(self.nodes[0].getbalance(minconf=0), 0) # wallet txs not in the mempool are untrusted
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(98.85983340 == 0)
```
https://cirrus-ci.com/task/4981266251513856?logs=ci#L3269
ACKs for top commit:
achow101:
ACK fae5bd9200
w0xlt:
ACK fae5bd9200
Tree-SHA512: 470f366720615c4a9326ec4c581fff569ecce9877f9134bb1975ec3d6f1d13a6403051418a91a80b2a86de617f43e539ec11bbf4f1713d0354d5b0ab98d22437
28ea4c7039 test: simplify splitment with `sendall` in wallet_basic (brunoerg)
923d24583d test: use `sendall` when emptying wallet (brunoerg)
Pull request description:
In some tests they have used `sendtoaddress` in order to empty a wallet. With the addition of `sendall`, it makes sense to use it for that.
ACKs for top commit:
achow101:
ACK 28ea4c7039
ishaanam:
utACK 28ea4c7039
w0xlt:
ACK 28ea4c7039
Tree-SHA512: 903136d7df5c65d3c02310d5a84241c9fd11070f69d932b4e188b8ad45c38ab5bc1bd5a9242b3e52d2576665ead14be0a03971a9ad8c00431fed442eba4ca48f
3add234546 ui: show header pre-synchronization progress (Pieter Wuille)
738421c50f Emit NotifyHeaderTip signals for pre-synchronization progress (Pieter Wuille)
376086fc5a Make validation interface capable of signalling header presync (Pieter Wuille)
93eae27031 Test large reorgs with headerssync logic (Suhas Daftuar)
355547334f Track headers presync progress and log it (Pieter Wuille)
03712dddfb Expose HeadersSyncState::m_current_height in getpeerinfo() (Suhas Daftuar)
150a5486db Test headers sync using minchainwork threshold (Suhas Daftuar)
0b6aa826b5 Add unit test for HeadersSyncState (Suhas Daftuar)
83c6a0c524 Reduce spurious messages during headers sync (Suhas Daftuar)
ed6cddd98e Require callers of AcceptBlockHeader() to perform anti-dos checks (Suhas Daftuar)
551a8d957c Utilize anti-DoS headers download strategy (Suhas Daftuar)
ed470940cd Add functions to construct locators without CChain (Pieter Wuille)
84852bb6bb Add bitdeque, an std::deque<bool> analogue that does bit packing. (Pieter Wuille)
1d4cfa4272 Add function to validate difficulty changes (Suhas Daftuar)
Pull request description:
New nodes starting up for the first time lack protection against DoS from low-difficulty headers. While checkpoints serve as our protection against headers that fork from the main chain below the known checkpointed values, this protection only applies to nodes that have been able to download the honest chain to the checkpointed heights.
We can protect all nodes from DoS from low-difficulty headers by adopting a different strategy: before we commit to storing a header in permanent storage, first verify that the header is part of a chain that has sufficiently high work (either `nMinimumChainWork`, or something comparable to our tip). This means that we will download headers from a given peer twice: once to verify the work on the chain, and a second time when permanently storing the headers.
The p2p protocol doesn't provide an easy way for us to ensure that we receive the same headers during the second download of peer's headers chain. To ensure that a peer doesn't (say) give us the main chain in phase 1 to trick us into permanently storing an alternate, low-work chain in phase 2, we store commitments to the headers during our first download, which we validate in the second download.
Some parameters must be chosen for commitment size/frequency in phase 1, and validation of commitments in phase 2. In this PR, those parameters are chosen to both (a) minimize the per-peer memory usage that an attacker could utilize, and (b) bound the expected amount of permanent memory that an attacker could get us to use to be well-below the memory growth that we'd get from the honest chain (where we expect 1 new block header every 10 minutes).
After this PR, we should be able to remove checkpoints from our code, which is a nice philosophical change for us to make as well, as there has been confusion over the years about the role checkpoints play in Bitcoin's consensus algorithm.
Thanks to Pieter Wuille for collaborating on this design.
ACKs for top commit:
Sjors:
re-tACK 3add234546
mzumsande:
re-ACK 3add234546
sipa:
re-ACK 3add234546
glozow:
ACK 3add234546
Tree-SHA512: e7789d65f62f72141b8899eb4a2fb3d0621278394d2d7adaa004675250118f89a4e4cb42777fe56649d744ec445ad95141e10f6def65f0a58b7b35b2e654a875
The test checks that parent txs are broadcast before child txs.
The previous behavior is that the rebroadcasting would simply iterate mapWallet. As
mapWallet is a std::unsorted_map, the child can sometimes come before the parent and thus
be rebroadcast in the wrong order and fail the test.
Both of these functions do almost the exact same thing. They can be
deduplicated so that their behavior matches except for the filtering
aspect. As this function will now always be called on wallet loading,
nNextResend will also always be initialized, so
wallet_resendwallettransactions.py is updated to account for that.
This also resolves a bug where ResendWalletTransactions would fail to
rebroadcast txs in insertion order thereby potentially rebroadcasting a
child transaction before its parent and causing the child to not
actually get rebroadcast.
Also names the combined function to ResubmitWalletTransactions as the
function just submits the transactions to the mempool rather than doing
any sending by itself.
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
Avoid permanently storing headers from a peer, unless the headers are part of a
chain with sufficiently high work. This prevents memory attacks using low-work
headers.
Designed and co-authored with Pieter Wuille.
5ef8c2c9fc test: fix typo for MaybeResendWalletTxs (stickies-v)
fbba4a1316 wallet: trigger MaybeResendWalletTxs() every minute (stickies-v)
Pull request description:
ResendWalletTransactions() only executes every [12-36h (24h average)](1420547ec3/src/wallet/wallet.cpp (L1947)). Triggering it every second is excessive, once per minute should be plenty.
The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review.
ACKs for top commit:
achow101:
ACK 5ef8c2c9fc
1440000bytes:
ACK 5ef8c2c9fc
Tree-SHA512: 4a077e3579b289c11c347eaa0d3601ef2dbb9fee66ab918d56b4a0c2e08222560a0e6be295297a74831836e001a997ecc143adb0c132faaba96a669dac1cd9e6
59aa54f731 i2p: log "SAM session" instead of "session" (Vasil Dimov)
d7ec30b648 doc: add release notes about the I2P transient addresses (Vasil Dimov)
47c0d02f12 doc: document I2P transient addresses usage in doc/i2p.md (Vasil Dimov)
3914e472f5 test: add a test that -i2pacceptincoming=0 creates a transient session (Vasil Dimov)
ae1e97ce86 net: use transient I2P session for outbound if -i2pacceptincoming=0 (Vasil Dimov)
a1580a04f5 net: store an optional I2P session in CNode (Vasil Dimov)
2b781ad66e i2p: add support for creating transient sessions (Vasil Dimov)
Pull request description:
Add support for generating a transient, one-time I2P address for ourselves when making I2P outbound connection and discard it once the connection is closed.
Background
---
In I2P connections, the host that receives the connection knows the I2P address of the connection initiator. This is unlike the Tor network where the recipient does not know who is connecting to them, not even the initiator's Tor address.
Persistent vs transient I2P addresses
---
Even if an I2P node is not accepting incoming connections, they are known to other nodes by their outgoing I2P address. This creates an opportunity to white-list given nodes or treat them differently based on their I2P address. However, this also creates an opportunity to fingerprint or analyze a given node because it always uses the same I2P address when it connects to other nodes. If this is undesirable, then a node operator can use the newly introduced `-i2ptransientout` to generate a transient (disposable), one-time I2P address for each new outgoing connection. That address is never going to be reused again, not even if reconnecting to the same peer later.
ACKs for top commit:
mzumsande:
ACK 59aa54f731 (verified via range-diff that just a typo / `unique_ptr` initialisation were fixed)
achow101:
re-ACK 59aa54f731
jonatack:
utACK 59aa54f731 reviewed range diff, rebased to master, debug build + relevant tests + review at each commit
Tree-SHA512: 2be9b9dd7502b2d44a75e095aaece61700766bff9af0a2846c29ca4e152b0a92bdfa30f61e8e32b6edb1225f74f1a78d19b7bf069f00b8f8173e69705414a93e
b21e522ce4 test: speedup wallet tests by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
In the course of testing #25297 by running all wallet-related functional tests (see https://github.com/bitcoin/bitcoin/pull/25297#issuecomment-1203365589), I noticed that the run-time of those tests vary a lot between runs, in fact too much for a useful comparison. This PR fixes this by making the tests both more deterministic and also faster, using the good ol' immediate tx relay trick (parameter `-whitelist=noban@127.0.0.1`).
master branch:
```
wallet_abandonconflict.py --descriptors | ✓ Passed | 7 s
wallet_abandonconflict.py --legacy-wallet | ✓ Passed | 23 s
wallet_balance.py --descriptors | ✓ Passed | 17 s
wallet_balance.py --legacy-wallet | ✓ Passed | 21 s
wallet_basic.py --descriptors | ✓ Passed | 32 s
wallet_basic.py --legacy-wallet | ✓ Passed | 56 s
wallet_bumpfee.py --descriptors | ✓ Passed | 44 s
wallet_bumpfee.py --legacy-wallet | ✓ Passed | 45 s
wallet_groups.py --descriptors | ✓ Passed | 89 s
wallet_groups.py --legacy-wallet | ✓ Passed | 94 s
wallet_hd.py --descriptors | ✓ Passed | 7 s
wallet_hd.py --legacy-wallet | ✓ Passed | 13 s
wallet_importdescriptors.py --descriptors | ✓ Passed | 26 s
wallet_listreceivedby.py --descriptors | ✓ Passed | 28 s
wallet_listreceivedby.py --legacy-wallet | ✓ Passed | 18 s
ALL | ✓ Passed | 520 s (accumulated)
Runtime: 526 s
```
PR branch:
```
wallet_abandonconflict.py --descriptors | ✓ Passed | 7 s
wallet_abandonconflict.py --legacy-wallet | ✓ Passed | 11 s
wallet_balance.py --descriptors | ✓ Passed | 8 s
wallet_balance.py --legacy-wallet | ✓ Passed | 8 s
wallet_basic.py --descriptors | ✓ Passed | 29 s
wallet_basic.py --legacy-wallet | ✓ Passed | 36 s
wallet_bumpfee.py --descriptors | ✓ Passed | 39 s
wallet_bumpfee.py --legacy-wallet | ✓ Passed | 32 s
wallet_groups.py --descriptors | ✓ Passed | 39 s
wallet_groups.py --legacy-wallet | ✓ Passed | 41 s
wallet_hd.py --descriptors | ✓ Passed | 8 s
wallet_hd.py --legacy-wallet | ✓ Passed | 11 s
wallet_importdescriptors.py --descriptors | ✓ Passed | 17 s
wallet_listreceivedby.py --descriptors | ✓ Passed | 7 s
wallet_listreceivedby.py --legacy-wallet | ✓ Passed | 9 s
ALL | ✓ Passed | 302 s (accumulated)
Runtime: 309 s
```
Note that an alternative approach could be to whitelist peers by default for nodes in the functional test framework and only enable the trickle relay for the few tests where it's really needed.
ACKs for top commit:
naumenkogs:
utACK b21e522ce4
Tree-SHA512: ac3c8f8f5a401d1b6af60ece9c77e72449f18920c2cb4a1bd65fb4d62cf428779ebf4e1d29009a882977b2252922df4e7183541e0da8de932f8cd479149e8a86
d1a0004621 test: add coverage for invalid parameters for `rescanblockchain` (brunoerg)
Pull request description:
This PR adds test coverage for the following errors:
2bd9aa5a44/src/wallet/rpc/transactions.cpp (L880-L894)
ACKs for top commit:
w0xlt:
reACK d1a0004621
Tree-SHA512: c357fbda3d261e4d06a29d2a5350482db5f97a815adf59abdac1971eb19b69cfd4d54e4d21836851e2e3b116aa2a820ea1437c7aededf86b06df435cca16ac90
1dc03dda05 [doc] remove non-signaling mentions of BIP125 (glozow)
32024d40f0 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow)
Pull request description:
We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy.
We should not use "BIP125" as synonymous with our RBF policy because:
- Our RBF policy is different from what is specified in BIP125, for example:
- the BIP does not mention our rule about the replacement feerate being higher (our Rule 6)
- the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380
- the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md
- the signaling policy is configurable, see #25353
- Our RBF policy may change further
- We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498
- See comments from people who are not me recently:
- https://github.com/bitcoin/bitcoin/pull/25038#discussion_r909507429
- https://github.com/bitcoin/bitcoin/pull/25575#issuecomment-1179519204
This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because:
- It is succint.
- It has already been widely marketed as BIP125 opt-in signaling.
- Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive.
- If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from.
Alternatives:
- Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6).
- Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.
- Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.
ACKs for top commit:
darosior:
ACK 1dc03dda05
ariard:
ACK 1dc03dda
t-bast:
ACK 1dc03dda05
Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
c3b099ace0 wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow)
116a620ce7 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow)
ff638323d1 test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow)
1bc8106d4c bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow)
31dd3dc9e5 bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow)
a0c3afb898 bumpfee: extract weights of external inputs when bumping fee (Andrew Chow)
612f1e44fe bumpfee: Calculate fee by looking up UTXOs (Andrew Chow)
Pull request description:
This PR allows `psbtbumpfee` to return a PSBT for transactions that contain external inputs. This does not work for bumping in the GUI nor `bumpfee` because these need private keys available to sign and send the transaction. But `psbtbumpfee` returns a psbt, so it is fine to not be able to sign.
In order to correctly estimate the size of the inputs for coin selection, the fee bumper will use the size of the inputs of the transaction being bumped. Because the sizes of signatures are not guaranteed, for external inputs, the fee bumper will verify the scripts with a special SignatureChecker which will compute the weight of all of the signatures in that input, and compute their weights if those signatures were maximally sized. This allows the fee bumper to obtain a max size estimate for each external input.
Builds on #23201 as it relies on the ability to pass weights in to coin selection.
Closes#23189
ACKs for top commit:
ishaanam:
reACK c3b099ace0
t-bast:
Re-ran my tests agains c3b099ace0, ACK
Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5
for verbose log messages for development or debugging only, as bitcoind may run
more slowly, that are more granular/frequent than the Debug log level, i.e. for
very high-frequency, low-level messages to be logged distinctly from
higher-level, less-frequent debug logging that could still be usable in production.
An example would be to log higher-level peer events (connection, disconnection,
misbehavior, eviction) as Debug, versus Trace for low-level, high-volume p2p
messages in the BCLog::NET category. This will enable the user to log only the
former without the latter, in order to focus on high-level peer management events.
With respect to the name, "trace" is suggested as the most granular level
in resources like the following:
- https://sematext.com/blog/logging-levels
- https://howtodoinjava.com/log4j2/logging-levels
Update the test framework and add test coverage.
If an external input's utxo was created by a transaction that the wallet
knows about, then it would not be selected using SelectExternal. This
results in either funding failure or incorrect weight calculation.
8cd21bb279 refactor: improve readability for AttemptSelection (josibake)
f47ff71761 test: only run test for descriptor wallets (josibake)
0760ce0b9e test: add missing BOOST_ASSERT (josibake)
db09aec937 wallet: switch to new shuffle, erase, push_back (josibake)
b6b50b0f2b scripted-diff: Uppercase function names (josibake)
3f27a2adce refactor: add new helper methods (josibake)
f5649db9d5 refactor: add UNKNOWN OutputType (josibake)
Pull request description:
This PR is to address follow-ups for #24584, specifically:
* Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult`
* Add missing `BOOST_ASSERT` to unit test
* Ensure functional test only runs if using descriptor wallets
* Improve readability of `AttemptSelection` by removing triple-nested if statement
Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.
ACKs for top commit:
achow101:
ACK 8cd21bb279
aureleoules:
ACK 8cd21bb279.
LarryRuane:
Concept, code review ACK 8cd21bb279
furszy:
utACK 8cd21bb2. Left a small, non-blocking, comment.
Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
a6b0c1fcc0 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot)
0fd2d14454 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot)
55f98d087e rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot)
b724476158 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot)
55a82eaf91 wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot)
Pull request description:
Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends).
It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit.
I'll add this to the output of `listunspent` too if this gets a few Concept ACKs.
ACKs for top commit:
instagibbs:
ACK a6b0c1fcc0
achow101:
re-ACK a6b0c1fcc0
Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
It's useful for an external application tracking coins to not be limited
by our change detection. For instance, for a watchonly wallet with two
descriptors a transaction from one to the other would be considered a
change output and not be included in the result (if the address was not
generated by this wallet).
The test is a bit primitive as it checks the Bitcoin Core log and
assumes that if it logs that it creates a transient session, then it
does that indeed.
A more thorough test would be to check that it indeed sends the
`SESSION CREATE ... DESTINATION=TRANSIENT` command and that it uses
the returned I2P address for connecting, even for repeated connections
to the same I2P peer. That would require a mocked SAM server (proxy)
implementation in Python.
fea75ad3ca refactor: Drop `boost/algorithm/string/replace.hpp` dependency (Hennadii Stepanov)
857526e8cb test: Add test case for `ReplaceAll()` function (Hennadii Stepanov)
Pull request description:
A new implementation of the `ReplaceAll()` seems enough for all of our purposes.
ACKs for top commit:
adam2k:
ACK Tested fea75ad3ca
theStack:
Code-review ACK fea75ad3ca
Tree-SHA512: dacfffc9d2bd1fb9f034baf8c045b1e8657b766db2f0a7f8ef7e25ee6cd888f315b0124c54aba7a29ae59186b176ef9868a8b709dc995ea215c6b4ce58e174d9
f6a916683d Add functional test for block announcements during initial headers sync (Suhas Daftuar)
05f7f31598 Reduce bandwidth during initial headers sync when a block is found (Suhas Daftuar)
Pull request description:
On startup, if our headers chain is more than a day behind current time, we'll pick one peer to sync headers with until our best headers chain is caught up (at that point, we'll try to sync headers with all peers).
However, if an INV for a block is received before our headers chain is caught up, we'll then start to sync headers from each peer announcing the block. This can result in doing a big headers sync with many (if not all) of our peers simultaneously, which wastes bandwidth.
This PR would reduce that overhead by picking (at most) one new peer to try syncing headers with whenever a new block is announced, prior to our headers chain being caught up.
ACKs for top commit:
LarryRuane:
ACK f6a916683d
ajtowns:
ACK f6a916683d
mzumsande:
ACK f6a916683d
dergoegge:
Code review ACK f6a916683d
achow101:
ACK f6a916683d
Tree-SHA512: 0662000bd68db146f55981de4adc2e2b07cbfda222b1176569d61c22055e5556752ffd648426f69687ed1cc203105515e7304c12b915d6270df8e41a4a0e1eaa
292b1a3e9c GetExternalSigner(): fail if multiple signers are found (amadeuszpawlik)
Pull request description:
If there are multiple external signers, `GetExternalSigner()` will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message, forcing the user to disconnect all but one external signer, so that there is no ambiguity as to which external signer was used.
ACKs for top commit:
Sjors:
tACK 292b1a3e9c
achow101:
ACK 292b1a3e9c
Tree-SHA512: e2a41d3eecc607d4f94e708614bed0f3545f7abba85f300c5a5f0d3d17d72c815259734accc5ca370953eacd290f27894ba2c18016f5e9584cd50fa1ec2fbb0b
8b3d2bbd0d test: add tests for `datacarrier` and `datacarriersize` options (w0xlt)
Pull request description:
As suggested in https://github.com/bitcoin/bitcoin/issues/25787, this PR adds tests for `datacarrier` and `datacarriersize` initialization options.
Close https://github.com/bitcoin/bitcoin/issues/25787.
ACKs for top commit:
theStack:
re-ACK 8b3d2bbd0d
stickies-v:
re-ACK 8b3d2bbd0d
Tree-SHA512: 962638ac9659f9d641bc5d1eff0571a08085dc7d4981b534b7ede03e4c702abd7048d543c199a588e2f94567b6d2393280e686629b19d7f4b24d365662be5e40
70a55c059b psbt: Avoid unsigned int overflow in PSBT_IN_TAP_BIP32_DERIVATION (Andrew Chow)
Pull request description:
Fixes#25749
ACKs for top commit:
instagibbs:
ACK 70a55c059b
darosior:
re-utACK 70a55c059b
jonatack:
Review ACK 70a55c059b, this should avoid the issue reported in https://github.com/bitcoin/bitcoin/issues/25749
Tree-SHA512: 6bb58e1cda9a5baa50fcd24f818b5b27ed94f0d33da3f71f6e457618176611bf2a84e1864e9a48d9303c301252bc4c1dee8b19a67dd713e849fb9442851ca341
b4a5ab96b4 test: refactor: deduplicate `DEFAULT_{ANCESTOR,DESCENDANT}_LIMIT` constants (Sebastian Falbesoner)
0fda1c7df6 scripted-diff: test: rename `MAX_{ANCESTORS,DESCENDANTS}` to `DEFAULT_{ANCESTOR,DESCENDANT}_LIMIT` (Sebastian Falbesoner)
Pull request description:
This PR renames the default in-mempool max ancestors/descendants constants `MAX_ANCESTORS`/`MAX_DESCENDANTS` in the functional tests to match the ones in the codebase:
c012875b9d/src/policy/policy.h (L58-L59)c012875b9d/src/policy/policy.h (L62-L63)
The custom limit constants `MAX_ANCESTORS_CUSTOM`/`MAX_DESCENDANTS_CUSTOM` are also renamed accordingly to better fit to this naming style. In the second commit, the default constants are deduplicated by moving them into the `messages.py` module. (Not sure if this module is really appropriate, as it doesn't have a connection to messages. If someone has a good suggestion, would be glad to hear it.)
ACKs for top commit:
w0xlt:
ACK b4a5ab96b4
glozow:
utACK b4a5ab96b4
fanquake:
ACK b4a5ab96b4
Tree-SHA512: a15c8256170afce3e383fceddcb562f588a02be97ce4202c84a2ebf22d73ab843f5e5a7d7c98e9ea044d8bcb7a4aeae0081d0e84c53e8fc0edffbcca00460139
4edc689382 doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner)
Pull request description:
As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by https://github.com/bitcoin/bitcoin/pull/25792#discussion_r941180819).
ACKs for top commit:
kouloumos:
ACK 4edc689382
1440000bytes:
ACK 4edc689382
w0xlt:
ACK 4edc689382
fanquake:
ACK 4edc689382
Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
0532aa7444 test: don't rely on usdt block_conn event order (0xb10c)
Pull request description:
Relying on block_connected event order in the USDT interface tests turned out to be brittle.
Closes https://github.com/bitcoin/bitcoin/issues/25793
Closes https://github.com/bitcoin/bitcoin/issues/25764
Top commit has no ACKs.
Tree-SHA512: 40b5012ac80a8eac9d2f374cd39304488009c29adb474dc5e8c03b96c354be358298d2ddee8de480afecc187e1bf42ee119b7aa6216b086a8bf77b7e1310213c
155344960b test: negative/unknown `rpcserialversion` should throw an init error (brunoerg)
Pull request description:
This PR adds test coverage for the following init errors:
41205bf442/src/init.cpp (L1025-L1030)
Top commit has no ACKs.
Tree-SHA512: 4456949e9a13908a5a59b13ed57bc3002b7ffd626e8dfb0346aa2600937ba1ef1b69cbae45cdb6bbc1c019dbcd64844e736a2ddd671a4704e53804355b6ea9f9
544b4332f0 Add wallet tests for spending rawtr() (Pieter Wuille)
e1e3081200 If P2TR tweaked key is available, sign with it (Pieter Wuille)
8d9670ccb7 Add rawtr() descriptor for P2TR with unknown tweak (Pieter Wuille)
Pull request description:
It may be useful to be able to represent P2TR outputs in descriptors whose script tree and/or internal key aren't known. This PR does that, by adding a `rawtr(KEY)` descriptor, where the KEY represents the output key directly. If the private key corresponding to that output key is known, it also permits signing with it.
I'm not convinced this is desirable, but presumably "tr(KEY)" sounds more intended for direct use than "rawtr(KEY)".
ACKs for top commit:
achow101:
ACK 544b4332f0
sanket1729:
code review ACK 544b4332f0
w0xlt:
reACK 544b4332f0
Tree-SHA512: 0de08de517468bc22ab0c00db471ce33144f5dc211ebc2974c6ea95709f44e830532ec5cdb0128c572513d352120bd651c4559516d4500b5b0a3d257c4b45aca
68006c10ab test: check that `verifymessage` RPC fails for non-P2PKH addresses (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `verifymessage` RPC, for the case that a non-P2PKH (but otherwise valid) address is passed:
e09ad284c7/src/util/message.cpp (L38-L40)e09ad284c7/src/rpc/signmessage.cpp (L48-L49)
The passed addresses to trigger the error are of the types nested segwit (P2SH-P2WPKH) and native segwit (P2WPKH) and are created with a helper function `addresses_from_privkey` using descriptors and the `deriveaddresses` RPC. At some point in the future, if we have BIP322 support, all those will likely succeed and can then be moved from error-throwing to the succedding assert loop.
ACKs for top commit:
achow101:
ACK 68006c10ab
w0xlt:
ACK 68006c10ab
Tree-SHA512: fec4ed97460787c2ef3d04e3fce89c9365c87207c8358b59c41890f3738355c002e64f289ab4aef794ef4dfd5c867be8b67d736fb620489204f2c6bfb8d3363c
db10cf8ae3 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm)
701a64f548 test: add support for Decimal to assert_approx (Karl-Johan Alm)
Pull request description:
(note: this was originally titled "add analyzerawtransaction RPC")
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
I originally proposed this to Elements (https://github.com/ElementsProject/elements/pull/1016) and it was suggested that I propose this upstream.
There is an alternative #22776 to instead add this info to `getbalances` when providing an optional transaction as argument.
ACKs for top commit:
jonatack:
ACK db10cf8ae3
achow101:
re-ACK db10cf8ae3
Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d
21a9e94dbb ci: remove hardcoded tag list from ci scripts (josibake)
d530ba390e doc: update test/README.md (josibake)
614d4682ba script: default to necessary tags in get_previous_releases.py (josibake)
Pull request description:
Almost every time I need to use this script, I forget the tag list is needed and that a specific set of tags is needed for the backwards compatibility tests to work. I end up wasting time reading through the script and googling to find the tag list before remembering it is in `test/README.md`
I assume (hope) I'm not the only one this happens to, so I figured it would make more sense to have the script default to downloading/building the necessary tags. This has the added benefit of making the script the source of truth: the script already needs to be updated with the SHA256_SUM of the binary for every new tag that is added, so it makes sense to use `SHA256_SUMS` list as the necessary tag list. This means there is less risk of the README and the script drifting (i.e updating the readme with a new tag and forgetting to update the script, or updating the script and forgetting to update the README). Now all that needs to happen is to update the `SHA256_SUMS` list in the script and everything Just Works (TM)
ACKs for top commit:
Sjors:
re-tACK 21a9e94dbb
Tree-SHA512: 97b488227a89a6827584edd251820a7074fad75dfd7f26f1aa5f858e2521d2e02effd0f11e6dc4676e1155d3d5aba6ff94a4b58ffef80dc201376afd5927deb9
This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally.
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
f2f6068b69 test: MiniWallet: add `send_self_transfer_chain` to create chain of txns (Andreas Kouloumos)
1d6b438ef0 test: use MiniWallet to simplify mempool_package_limits.py tests (Andreas Kouloumos)
Pull request description:
While `wallet.py` includes the MiniWallet class and some helper methods, it also includes some methods that have been moved there without having any direct relation with the MiniWallet class. Specifically `make_chain`, `create_child_with_parents` and `create_raw_chain` methods that were extracted from `rpc_packages.py` at f8253d69d6 in order to be used on both `mempool_package_limits.py` and `rpc_packages.py`.
Since that change, due to the introduction of additional methods in MiniWallet, the functionality of those methods can now be replicated with the existing MiniWallet methods and simultaneously simplify those tests by using the MiniWallet.
This PR's goals are
- to simplify the `mempool_package_limits.py` functional tests with usage of the MiniWallet.
- to make progress towards the removal of the `make_chain`, `create_child_with_parents` and `create_raw_chain` methods of `wallet.py`.
For the purpose of the aforementioned goals, a helper method `MiniWallet.send_self_transfer_chain` is introduced and method `bulk_transaction` has been integrated in `create_self_transfer*` methods using an optional `target_weight` option.
ACKs for top commit:
MarcoFalke:
ACK f2f6068b69👜
Tree-SHA512: 3ddfa0046168cbf7904ec6b1ca233b3fdd4f30db6aefae108b6d7fb69f34ef6fb2cf4fa7cef9473ce1434a0cc8149d236441a685352fef35359a2b7ba0d951eb
Moved `bulk_transaction` into MiniWallet class as `_bulk_tx` private
helper method to be used when the newly added `target_weight` option is
passed to `create_self_transfer*`
395767e9f1 Add test case mimicking issue 24765 (Pieter Wuille)
Pull request description:
This adds a functional test for the concern brought up in #24765. It turned out to be a non-issue, but since I wrote it anyway, it can't hurt to add it.
Top commit has no ACKs.
Tree-SHA512: fc8d57129d8c68f6d9a41b94b5ff676c87b31f53bc958195d4fe312530ec3e038ebd0bc5e8b9d56be77b7b63fd94574685901901404a4ab8726a5e09d89e86c8
cc7335edc8 ci: run USDT interface test in a VM (0xb10c)
dba6f82342 test: adopt USDT utxocache interface tests (0xb10c)
220a5a2841 test: hook into PID in tracing tests (0xb10c)
Pull request description:
Changes a CI task that runs test the previously not run `test/functional/interface_usdt_*.py` functional tests (added in https://github.com/bitcoin/bitcoin/pull/24358).
This task is run as CirussCI `compute_engine_instance` VM as hooking into the tracepoints is not possible in CirrusCI docker containers (https://github.com/bitcoin/bitcoin/issues/23296#issuecomment-1024920845). We use an unoffical PPA and untrusted `bpfcc-tools` package in the CI as the Ubuntu jammy and Debian bullseye packages are outdated. We hope use an official package when new Ubuntu/Debian releases are available for the use with Google Compute Engine.
We make sure to hook into `bitcoind` binaries in USDT interface tests via their PID, instead of their path. This makes sure multiple functional tests running in parallel don't interfere with each other.
The utxocache USDT interface tests is adopted to a change of the functional test framework that wasn't detected as the tests weren't run in the CI. As the tracepoints expose internals, it can happen that we need to adopt the interface test when internals change. This is a bit awkward, and if it happens to frequently, we should consider generalizing the tests a bit more. For now it's fine, I think.
See the individual commit messages for more details on the changes.
Fixes https://github.com/bitcoin/bitcoin/issues/24782
Fixes https://github.com/bitcoin/bitcoin/issues/23296
I'd like to hear from reviewers:
- Are we OK with using the [`hadret/bpfcc`](https://launchpad.net/~hadret/+archive/ubuntu/bpfcc) PPA for now? There is a clear plan when to drop it and as is currently, it could only impact the newly added VM task.
- ~~Adding a new task increases CI runtime and costs. Should an existing `container` CI task be ported to a VM and reused instead?~~ Yes, see https://github.com/bitcoin/bitcoin/pull/25528#issuecomment-1179509525
ACKs for top commit:
MarcoFalke:
cr ACK cc7335edc8
Tree-SHA512: b7fddccc0a77d82371229d048abe0bf2c4ccaa45906497ef3040cf99e7f05561890aef4c253c40e4afc96bb838c9787fae81c8454c6fd9db583276e005a4ccb3
ab3c06db1a doc: Release notes for default RBF (Andrew Chow)
61d9149e78 rpc: Default rbf enabled (Andrew Chow)
e3c33637ba wallet: Enable -walletrbf by default (Andrew Chow)
Pull request description:
The GUI currently opts in to RBF by default, but RPCs do not, and `-walletrbf` is default disabled. This PR makes the default in those two places to also opt in.
The last time this was proposed (#9527), the primary objections were the novelty at the time, the inability to bump transactions, and the gui not having the option to disable rbf. In the 5 years since, RBF usage has steadily grown, with ~27% of txs opting in. The GUI has the option to enable/disable RBF, and is also defaulted to having it enabled. And we have the ability to bump RBF'd transactions in both the RPC and the GUI. So I think it makes sense to finally change the default to always opt in to RBF.
ACKs for top commit:
darosior:
reACK ab3c06db1a
aureleoules:
ACK ab3c06db1a.
glozow:
utACK ab3c06db1a
Tree-SHA512: 81b012c5033e270f86a87a6a196ccc549eb54b158eebf88e917cc6621d40d7bdcd1566b602688907dd5d364b95a557b29f97dce869cea512e339588262c027b6
71d1d13627 test: add unit test for AvailableCoins (josibake)
da03cb41a4 test: functional test for new coin selection logic (josibake)
438e04845b wallet: run coin selection by `OutputType` (josibake)
77b0707206 refactor: use CoinsResult struct in SelectCoins (josibake)
2e67291ca3 refactor: store by OutputType in CoinsResult (josibake)
Pull request description:
# Concept
Following https://github.com/bitcoin/bitcoin/pull/23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern.
## Leaking information in a later transaction
Consider the following scenario:
1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address
2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a`
3. Alice then pays Carol, who gives her a bech32 address
4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs
From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction.
**TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so.
# Approach
`AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior.
I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`.
ACKs for top commit:
achow101:
re-ACK 71d1d13627
aureleoules:
ACK 71d1d13627.
Xekyo:
reACK 71d1d13627 via `git range-diff master 6530d19 71d1d13`
LarryRuane:
ACK 71d1d13627
Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426
4e616d20c9 test: check that combining PSBTs with different txs fails (Sebastian Falbesoner)
2a428c7989 test: support passing PSBTMaps directly to PSBT ctor (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `combinepsbt` RPC, in the case of combining two PSBTs with different transactions:
b8067cd435/src/psbt.cpp (L24-L27)
The calling function `CombinePSBTs` checks for the false return value and then returns the transaction error string `PSBT_MISMATCH`:
b8067cd435/src/psbt.cpp (L433-L435)b8067cd435/src/util/error.cpp (L30-L31)
ACKs for top commit:
instagibbs:
reACK 4e616d20c9
achow101:
ACK 4e616d20c9
Tree-SHA512: 45b2b224b13b44ad69ae62e4bc20f74cab32770cf8127b026ec47a7520f7253148fdbf1fad612afece59e45a6738bef9a351ae87ea98dc83d095cc78f6db0318
71a751f6c3 test: add test for decoding PSBT with per-input preimage types (Sebastian Falbesoner)
faf43378e2 refactor: move helper `random_bytes` to util library (Sebastian Falbesoner)
fdc1ca3896 test: add constants for PSBT key types (BIP 174) (Sebastian Falbesoner)
1b035c03f9 refactor: move PSBT(Map) helpers from signet miner to test framework (Sebastian Falbesoner)
7c0dfec2dd refactor: move `from_binary` helper from signet miner to test framework (Sebastian Falbesoner)
597a4b35f6 scripted-diff: rename `FromBinary` helper to `from_binary` (signet miner) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `decodepsbt` RPC in the case that a PSBT with on of the per-input preimage types (`PSBT_IN_RIPEMD160`, `PSBT_IN_SHA256`, `PSBT_IN_HASH160`, `PSBT_IN_HASH256`; see [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#Specification)) is passed. As preparation, the first four commits move the already existing helpers for (de)serialization of PSBTs and PSBTMaps from the signet miner to the test framework (in a new module `psbt.py`), which should be quite useful for further tests to easily create PSBTs.
ACKs for top commit:
achow101:
ACK 71a751f6c3
Tree-SHA512: 04f2671612d94029da2ac8dc15ff93c4c8fcb73fe0b8cf5970509208564df1f5e32319b53ae998dd6e544d37637a9b75609f27a3685da51f603f6ed0555635fb
in order to run the backwards compatibility tests, specific releases are needed.
previously, the list of tags was in test/README.md, but it makes more sense to
have them as the default list in script
facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
7878f97bf1 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky)
ee3a079fab indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
dc971be083 indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky)
bef4e405f3 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky)
addb4f2af1 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky)
33b4d48cfc indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky)
a0b5b4ae5a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky)
Pull request description:
Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes.
This PR contains the first 7 commits from https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1165625977 which have been split off for easier review. Previous review comments can be found in #24230
ACKs for top commit:
MarcoFalke:
ACK 7878f97bf1 though did not review the last commit 🤼
mzumsande:
Code Review ACK 7878f97bf1
Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989
Create a wallet with mixed OutputTypes and send a volley of payments,
ensuring that there are no mixed OutputTypes in the txs. Finally,
verify that OutputTypes are mixed only when necessary.
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
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
ea54ba2f42 [test] Fix port collisions caused by p2p_getaddr_caching.py (dergoegge)
f9682e75ac [test_framework] Set PortSeed.n directly after initialising params (dergoegge)
Pull request description:
This PR fixes the issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/25096#discussion_r892558783), to avoid port collisions between nodes spun up by the test framework.
Top commit has no ACKs.
Tree-SHA512: ec9159f0af90db636f7889d664c24e1430cf2bcb3c02a9ab2dcfe531b2a4d18f6e3a0f8ba73071bdf2f7db518df9d5d86a9cd06695e67644d20fe4515fac32b7
If there are multiple external signers, `GetExternalSigner()` will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message.
d575413fb8 doc: add `desig` to ignore-words (brunoerg)
c06cc41ddb doc: fix typo in kernel/context.h (brunoerg)
Pull request description:
This PR fixes a typo in `kernel/context.h` (libary => library) and add `desig` to ignore-words since it's a valid word, see:
b9416c3847/src/net.cpp (L1105-L1117)
ACKs for top commit:
fanquake:
ACK d575413fb8
Tree-SHA512: 2d548c737b8184d0243445c7503f3f68256ecb0970bd834d52de099de3cd8c8b9c140e2b77d55e2542fbd45b1d21cbdee639f5b2ef8138c37b8b72e5211029c3
fa74b63c01 test: Fix wait_for_debug_log UnicodeDecodeError (MacroFake)
Pull request description:
Fix the intermittent `UnicodeDecodeError` when the debug log is truncated on an (multi-byte) unicode character by treating everything as bytes.
Also, remove the `ignore_case` option and the`re.search+re.escape` wrap. All of this is unused and doesn't exist on raw byte strings.
Fixes https://github.com/bitcoin/bitcoin/issues/24575
ACKs for top commit:
jonatack:
ACK fa74b63c01
brunoerg:
ACK fa74b63c01
Tree-SHA512: c67c9355073e784fa8d9d48b8e79ff0c98f5ae9cd4d704ad12a76d2604733946054bc74b8ab346aa2184db23d740b85c8c13eb892d76cba92e42ebfd73f2f1bf
292828cd77 [test] Test addr cache for multiple onion binds (dergoegge)
3382905bef [net] Seed addr cache randomizer with port from binding address (dergoegge)
f10e80b6e4 [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge)
Pull request description:
The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): a8098f2cef/src/net.cpp (L2800-L2804)
For inbound onion connections `CNode::addr.GetNetwork()` returns `NET_UNROUTABLE` and `CNode::addrBind` is set to `127.0.0.1:<onion bind port>`. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.
To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using `CNode::ConnectedThroughNetwork()`) as well as the port of `CNode::addrBind`.
ACKs for top commit:
sipa:
utACK 292828cd77
mzumsande:
Code Review ACK 292828cd77
naumenkogs:
utACK 292828cd77
Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f
433b525694 Add LogPrintLevel to lint-format-strings, drop LogPrint-vs-LogPrintf section in dev notes (Jon Atack)
Pull request description:
added by #7003 in 2015, as that potential issue would now be caught by the `test/lint/lint-format-strings.py` script run by the CI.
ACKs for top commit:
MarcoFalke:
cr ACK 433b525694
w0xlt:
ACK 433b525694
Tree-SHA512: 91a2ac76689ed4f1f638e07c16d2ec8952fb013cc8bb896780fbd9333abd084281ce99afdc9de715d07a9abb4dce5dd67edf5e347aff466c6ef339ccc4158679
687addaf13 test: add BIP-125 rule 5 testcase with default mempool (James O'Beirne)
6120e8e287 test: allow passing sequence through create_self_transfer_multi (James O'Beirne)
Pull request description:
Currently, we only test rule 5 of BIP-125 (replacement transactions cannot evict more than 100 transactions) by changing default mempool parameters to allow for more descendants. The current test works on a single transaction graph that has over 100 descendants.
This patch adds a test to exercise rule 5 using the default mempool parameters. The case is a little more sophisticated: instead of working on a single transaction graph, it uses a replacement transaction to "unite" several UTXOs which join independent transaction graphs. The total number of transactions in these graphs sum to more than the max allowable replacement.
I think the difference in transaction topology makes this a worthwhile testcase to have, setting aside the fact that this testcase works without having to use atypical mempool params.
See also: [relevant discussion from IRC](https://www.erisian.com.au/bitcoin-core-dev/log-2022-05-27.html#l-126)
ACKs for top commit:
laanwj:
Code review ACK 687addaf13
LarryRuane:
ACK 687addaf13
Tree-SHA512: e589aeaf9d6f137d546b7809f8795d6f6043d87b15e97c2efe85b42ce8b49d977ee7d79440c542ca4b0b5ca2de527488029841a1ffc0d96c5771897df4b3f324
that was added in 2015 by commit b8c06ef40 in PR 7003, as that potential issue
would now be caught by the test/lint/lint-format-strings.py script run by the CI
f26a496dfd test: clean up all-lint.py (Martin Leitner-Ankerl)
64d72c4c87 test: rename lint-all.py to all-lint.py (Martin Leitner-Ankerl)
Pull request description:
When running `./test/lint/lint-all.py`, the script runs all tests but
also calls itself because the comparison with `__file__` doesn't work.
Comparing resolved paths gives reliable comparison, and lint-all.py doesn't call itself any more
ACKs for top commit:
laanwj:
Code review ACK f26a496dfd
Tree-SHA512: b44abdd685f7b48a6a9f48e96d97138b635c31c1c7ab543cb5636b5f49690ccd56fa6fec01ae7fcc16af01a613372ee77632f70c32059919b373aa8051953791
e593ae07c4 Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block (Luke Dashjr)
Pull request description:
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.
~~(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)~~
ACKs for top commit:
fjahr:
utACK e593ae07c4
ryanofsky:
Code review ACK e593ae07c4. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!
Tree-SHA512: c2d511df80682d57260aae8af1665f9d7eaed16448f185f4c9f23c78fa9b8289a02053da7a0b83643fef57610d601ea63b59ff39661a51f4827f1eb27cc30594
3a9b9bb38e test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes#25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e
laanwj:
Code review ACK 3a9b9bb38e
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last
block pruned was returned, subject to a bug if there were blocks left unpruned
due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new
bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it
makes more sense to fix the behaviour to match the documentation.
48262a00f5 Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bb Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f5
sipa:
ACK 48262a00f5
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
This testcase exercises rule 5 of BIP-125 (no more than 100 evictions
due to replacement) without having to test under non-default mempool
parametmers.
1da5e45725 test: use MiniWallet for feature_dbcrash.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_dbcrash.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
ACKs for top commit:
laanwj:
Code review ACK 1da5e45725
brunoerg:
crACK 1da5e45725
Tree-SHA512: 75ee9a32fd1451254004797d695d18032bd0fcb66ebd88cf737e147e43812525f6e884ec05fcc4f76f566dc71174c8ed7347bcdce16567db6511746ae64cead0
f565b2836d Fixup option name in bench message (Ben Woosley)
bf209ac7a7 doc: Fix spelling errors identified by codespell in coments (Ben Woosley)
Pull request description:
From the output [here](https://cirrus-ci.com/task/5275612980969472?logs=lint#L849):
```
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
~~I left the 'nd' in miniscript_tests as-is, as it's valid miniscript,
and I'm wary of whitelisting it.~~
ACKs for top commit:
dunxen:
ACK f565b28
Tree-SHA512: 501a426c5f6f9761e2c8f980d5d955611428a827321888f53e0ae9526b0fecd43f9d1fa845fc70ae2489d77be6dc0b5b371dff55c5146f4b39ed874f4a1ea917
a35f963edf Add test for getheaders behavior (Suhas Daftuar)
ef6dbe6863 Respond to getheaders if we have sufficient chainwork (Suhas Daftuar)
Pull request description:
Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time.
To make things simpler to reason about, just use `nMinimumChainWork` as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).
ACKs for top commit:
klementtan:
crACK a35f963edf
naumenkogs:
ACK a35f963edf
MarcoFalke:
review ACK a35f963edf 🗯
Tree-SHA512: 131e3872e7fe80382ea9c1ec202d6c2dc59c006355c69000aa3f4ce6bccd02a6c689c8cb8f3542b5d9bc48bfa61edcbd1a78535c0b79018971d02bed2655d284
e8959000b6 test: Use MiniWallet in rpc_rawtransaction.py (Daniela Brozzoni)
e93046c10b MOVEONLY: Move signrawtransactionwithwallet test (Daniela Brozzoni)
Pull request description:
This PR allows `rpc_rawtransaction.py` to be run even without the Core wallet by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`. Since this would have meant running the same test twice
if the wallet wasn't compiled, now we run it just once with the legacy
wallet.
ACKs for top commit:
jonatack:
ACK e8959000b6
Tree-SHA512: d1580570a54dad8e30a5df1ab7d03ecb3f824efe6843323e1f3aef63592045d823c7d54fc86321dc7c1d414854a253431a01a7baa9f30426ea9a09ef11ae3a04
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`.
Now we run it only with `--legacy-wallet`, as all the tests has been
ported to the MiniWallet but `raw_multisig_transaction_legacy_tests`,
which can be run only with the legacy wallet.
We also decrease the number of nodes used from 4 to 3, making the test
run slightly faster.
Put signrawtransactionwithwallet_tests in rpc_signrawtransaction.py,
as the test is mainly testing the signrawtransaction RPC.
Review with `git show --color-moved=dimmed-zebra`
4185570340 Add RPC to get mempool txs spending outputs (t-bast)
Pull request description:
We add an RPC to fetch mempool transactions spending any of the given outpoints.
Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool).
For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly.
I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it.
ACKs for top commit:
darosior:
re-utACK 4185570340
vincenzopalazzo:
re-ACK 4185570340
danielabrozzoni:
re-tACK 4185570340
w0xlt:
reACK 4185570340
Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc
295ff61934 test: add coverage for unknown -blockfilterindex (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
44037a2912/src/init.cpp (L844)
Passing an unknown value to -blockfilterindex should throw an error.
ACKs for top commit:
dunxen:
cr-ACK 295ff61
Tree-SHA512: 1444903cf0696406c485ce0575f951d527fe7d699094d5845622c0b57c954d6d7dcf1e78ef0c4e8b9b26f53b79583f07fec0e8d8e7f04aa744d2a8cd98329db9
From the output here:
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
https://cirrus-ci.com/task/5275612980969472?logs=lint#L849
I added 'nd' to the spelling.ignored-words.txt, as it's valid miniscript.
664a14ba7c coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687566 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f04 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c9 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6 coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b8 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993 kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)
Pull request description:
Part of: #24303
Depends on: #24322
The `GetUTXOStats` function has 2 codepaths:
- One which queries the `CoinStatsIndex` for the UTXO hash
- One which actually performs the hashing
For `libbitcoinkernel`, the only place where we call `GetUTXOStats` is in `PopulateAndValidateSnapshots`, which uses the `SHA256D` hash, and is therefore unable to use the `CoinStatsIndex` since that only provides `MuHash` hashes. Not that I think indices necessarily belong in `libbitcoinkernel` anyway.
This PR separates these 2 aforementioned codepaths of `GetUTXOStats`, uses the hashing codepath in `PopulateAndValidateSnapshots`, and removes the need to link in `index/coinstatsindex.cpp` and `node/coinstats.cpp`.
-----
Logistically, this PR:
- Extracts out the `index_requested` and `hash_type` members of `CoinStats`, which served as "in-params" to `GetUTXOStats` embedded within the `CoinStats` struct. This allows `CoinStats` to only consist of "out-param" members, and be returned by `GetUTXOStats` without needing to be an "in-out" param
- Introduce the purely virtual `UTXOHashers` class, with 3 implementations: `SHA256DHasher`, `MuHashHasher`, and `NullHasher`. These replace the existing template-based polymorphism.
- Split `GetUTXOStats` into:
- `CalculateUTXOStatsWithHasher(UTXOHasher&, ...)`, and
- `LookupUTXOStatsWithIndex(CoinStatsIndex&, ...)`
- Use `CalculateUTXOStatsWithHasher` directly where appropriate (`src/validation.cpp` and `src/fuzz`)
- Move `GetUTXOStats` to `rpc/blockchain`, which is the only place that depends on `GetUTXOStats`'s weird fallback behaviour
- Move `LookupUTXOStatsWithIndex` to `index/coinstatsindex`
Code organization:
- `src/`
- `kernel/` → only contains the hashing codepath
- `coinstats.cpp` → hashing codepath implementations
- `coinstats.h` → header for `kernel/coinstats.cpp`
- `index/` → only contains the index codepath
- `coinstatsindex.cpp` → index codepath implementations
- `coinstatsindex.h`
- `validation.cpp` → only uses the hashing codepath
- `rpc/blockchain.cpp` → uses both the hashing and index codepath, old `GetUTXOStats` fallback logic moved here as static
- `test/fuzz/coins_view.cpp` → only uses the hashing codepath
TODOs:
- [x] Commit messages could be fleshed out more
Would love any feedback!
ACKs for top commit:
laanwj:
Code review ACK 664a14ba7c
Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73
This is the "fruit of our labor" for this patchset.
ChainstateManager::PopulateAndValidateSnapshot can now directly call
ComputeUTXOStats(...).
Our consensus engine is now fully decoupled from all indices.
See the src/Makefile.am for some satisfying removals.
908fb7e2ec test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80a74 test: Don't use shell=True in `lint-files.py` (laanwj)
Pull request description:
Improvements to the `lint-files.py` script:
- Avoid use of `shell=True`.
- Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`.
(what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally).
ACKs for top commit:
vincenzopalazzo:
re-tACK 908fb7e2ec
Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
baa3ddc49c doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)
Pull request description:
Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.
If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).
ACKs for top commit:
achow101:
ACK baa3ddc49c
theStack:
re-ACK baa3ddc49c
w0xlt:
ACK baa3ddc49c
Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
a4703ce9d7 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner)
Pull request description:
Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK a4703ce9d7
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
1d4122dfef init: Allow -proxy="" setting values (Ryan Ofsky)
Pull request description:
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with https://github.com/bitcoin/bitcoin/pull/15936, reported in https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.
ACKs for top commit:
hebasto:
re-ACK 1d4122dfef, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
055d94d1ab test: add coverage for unknown network in -onlynet (brunoerg)
Pull request description:
This PR adds test coverage for the following init error by passing an unknown network in -onlynet
0de36941ec/src/init.cpp (L1311)
ACKs for top commit:
MarcoFalke:
rACK 055d94d1ab
Tree-SHA512: 01bbb297afff371f6345889fa04117ff195b68f0bbf934878ba446049791fdbd7d2ce119ee4f9b3616cc0a81330d7055507dc81151acf68532c077f3575258e9
bf6526f4a0 [test] Remove segwit argument from build_block_on_tip() (John Newbery)
c65bf50b44 Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery)
Pull request description:
This implements two of the suggestions from code reviews of PR 20799:
- Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
- Remove segwit argument from build_block_on_tip()
ACKs for top commit:
dergoegge:
Code review ACK bf6526f4a0
naumenkogs:
ACK bf6526f4a0
Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
5dc6d92077 test: make BIP157 messages default-constructible (MESSAGEMAP compatibility) (Sebastian Falbesoner)
71e4cfefe7 test: p2p: add missing BIP157 message types to MESSAGEMAP (Sebastian Falbesoner)
Pull request description:
The script [message-capture-parser.py](https://github.com/bitcoin/bitcoin/blob/master/contrib/message-capture/message-capture-parser.py) currently doesn't support parsing the BIP157 messages `getcfilters`, `getcfheaders` and `getcfcheckpt`, e.g.
```
$ ./contrib/message-capture/message-capture-parser.py msgs_recv.dat
...
WARNING - Unrecognized message type b'getcfcheckpt' in /home/thestack/bitcoin/msgs_recv.dat
...
```
This PR fixes this by adding the missing message type mappings to the [`MESSAGEMAP`](225e5b57b2/test/functional/test_framework/p2p.py (L95-L127)) in the test framework and add default-constructors for the corresponding `msg_`... classes.
Without the second commit, the following error message would occur:
```
File "/home/thestack/bitcoin/./contrib/message-capture/message-capture-parser.py", line 141, in process_file
msg = MESSAGEMAP[msgtype]()
TypeError: __init__() missing 2 required positional arguments: 'filter_type' and 'stop_hash'
```
ACKs for top commit:
dunxen:
tACK [5dc6d92](5dc6d92077)
Tree-SHA512: d656c4d38a856373f01d7c293ae7d2b27378a9fc248048ebf2a64725ef8b498b3ddf4f420704abdb20d0c68ca548f1777602c5e73b66821a20c97ae618f1d63f
faac67cab0 test: Fix intermittent race in p2p_unrequested_blocks.py (MacroFake)
Pull request description:
Disconnect may also result in an `OSError`, not only an `AssertionError`. Instead of maintaining a dead code path and enumerating disconnect reasons, just assume disconnection happens every time.
ACKs for top commit:
jamesob:
Code review ACK faac67cab0
Tree-SHA512: d2cec003168e421a5faed275cb2e1ef9fc63f9e8514f41d21da17e8964c79e5b453ccd72cd7ec62805f45293cf877be5bc8124ae98a515c0aa42d6e053409653
The only place that segwit=True is for a block that contains only the
coinbase transaction. Since the witness commitment is optional if none
of the transactions have a witness, we can leave it out. This doesn't
change the test coverage, which is testing p2p compact block logic.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#discussion_r867782119
ada8358ef5 Sanitize port in `addpeeraddress()` (amadeuszpawlik)
Pull request description:
In connection to #22087, it has been [pointed out](https://github.com/bitcoin/bitcoin/pull/22087#pullrequestreview-674786285) that `addpeeraddress` needs to get its port-value sanitized.
ACKs for top commit:
fanquake:
ACK ada8358ef5
Tree-SHA512: 48771cd4f6940aa7840fa23488565c09dea86bd5ec5a5a1fc0374afb4857aebcd2a1f51e2d4cb7348460e0ad9793dc5d2962df457084ed2b8d8142cae650003f
Subsequent commits will remove support for other versions of compact blocks.
Add a test that a received `sendcmpct` message with version = 1 is
ignored.
4faa550072 test: Fix race condition in index pruning test (Fabian Jahr)
Pull request description:
Fixes#25031
The `feature_index_prune.py` test seems to be racy because connections are reestablished after restarts and the blocks are synced via the `sync_blocks` function. The `sync_blocks` function has a sanity check at the beginning to check that all nodes in the set have at least one established connection and that is not always the case.
As a solution nodes are not connected via the `-connect` parameter on start but instead via the `connect_nodes` helper.
Top commit has no ACKs.
Tree-SHA512: f88377715f455f1620725fe8ebd6b486fa0209660b193bf68d1ce1452e2086ac5d169d8ca4c2b61443566232e96fb9c6386ee482bc546cce38078d72e7c3c29f
Nodes are restarted and reconnected as part of the test. Afterwards
`sync_blocks` is called immediately on the nodes. `sync_blocks`
first checks that all the included nodes have at least one
connection. Since adding a connection is usually happening in a
thread, sometimes nodes could run into this check before the
connection was fully established so that it would fail the entire
test.
This fix uses the `connect_nodes` helper to make the connection the
nodes. `connect_nodes` has a wait for the connection built into it.
In order to deserialize received or read messages via lookup in
MESSAGEMAP (e.g.: `t = MESSAGEMAP[msgtype]()`), the messages must have a
default constructor, i.e. there needs to be the possibility to
initialize them with zero arguments.
faa5a7a573 test: Check msg type in msg capture is followed by zeros (MacroFake)
Pull request description:
Checking that they are not printable is an odd (and wrong) way to check that all chars are zero.
ACKs for top commit:
theStack:
Code-review ACK faa5a7a573
Tree-SHA512: 63e001bd25298dcf47606f8ab11ddfb704ca963304149b0f6e188eb7dcf45c41f92d39f26bda32bceb03384720c9bdddb2673dba513cd9242dc9663d498b3f29
This change improves the usability of the `dumptxoutset` RPC in two ways,
in the case that an invalid path is passed:
1. return from the RPC immediately, rather then when the file is first
tried to be written (which is _after_ calculating the UTXO set hash)
2. return a proper return code and error message instead of the cryptic
"CAutoFile::operator<<: file handle is nullptr: unspecified
iostream_category error" (-1)
3258bad996 changes color of skipped functional tests (Jacob P. Fickes)
Pull request description:
changes the color of skipped functional tests (currently grey and can be hard to read/invisible on dark backgrounds) to yellow.
resolves#24791
ACKs for top commit:
theStack:
Tested ACK 3258bad996
jarolrod:
Tested ACK 3258bad996
Tree-SHA512: 3fe5ae0d3b4902b2b6bda6e89ab780feb8bf4b7cb1ce7e8467057b94a1e0a26ddeaf3cac0bc19b06ef10d8bccaac9c495029d42740fbedab8fb0d5fdd7d02eaf
These are unreferenced in the CI and documentation, and have been since
2019 (see #17549).
I'm not sure the cppcheck is worthwhile. It takes a long time
to run (I think this is why it isn't in the normal lints), and right
now it only appears to find implicit constructors. The list of
exceptions is out of date. But if anyone wants to bring it back at any
time in the future they can do so from git history (and port it to Python).
dba1231672 test: previous releases: add v23.0 (Sjors Provoost)
Pull request description:
Follows the same pattern as d8b705f1ca (v22.0) and 8a57a06a50 (v0.21.0).
Starting from v23.0 there is a separate macOS release for x86_64 and aarch64.
ACKs for top commit:
prusnak:
Approach ACK dba1231672
Tree-SHA512: 249aeddd5e80e163578581e5c8e9b6579f3694abc3d1fb68dddb7b42d75021ad85266688ec4a365a6631d82a65a19873aff7ba61c0ea59d21f8adbe4b772dc16
bd6ceb4049 test: port 'lint-shell.sh' to python (whiteh0rse)
Pull request description:
Converts `test/lint/lint-shell.sh` to Python and updates the docs accordingly. In order for the linter to run, it requires `git` and the `shellcheck` linter to be installed on the system. The script will fail gracefully with a help message if `shellcheck` is not installed.
Top commit has no ACKs.
Tree-SHA512: edc3f1af582b736a0b46f32bd7448e859201dc43f5dd086f16aab49037a1ab936f5376c29fc1006a932b9e98b4f2423d83d98e9666304781a06eb4d2a16f54e3
We add an RPC to fetch the mempool transactions spending given outpoints.
Without this RPC, application developers would need to first call
`getrawmempool` which returns a long list of `txid`, then fetch each of
these txs individually to check whether they spend the given outpoint(s).
This RPC can later be enriched to also find confirmed transactions instead
of being restricted to mempool transactions.
e3a06a3c6c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7 util: Refactor SysErrorString logic (laanwj)
e7f2f77756 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbf util: Replace non-threadsafe strerror (laanwj)
Pull request description:
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in #4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives (with code from `NetworkErrorString`) and replace all uses of `strerror` with this.
Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.
Edit2: from the linux manpage:
```
ATTRIBUTES
For an explanation of the terms used in this section, see attributes(7).
┌───────────────────┬───────────────┬─────────────────────────┐
│Interface │ Attribute │ Value │
├───────────────────┼───────────────┼─────────────────────────┤
│strerror() │ Thread safety │ MT-Unsafe race:strerror │
├───────────────────┼───────────────┼─────────────────────────┤
…
├───────────────────┼───────────────┼─────────────────────────┤
│strerror_r(), │ Thread safety │ MT-Safe │
│strerror_l() │ │ │
└───────────────────┴───────────────┴─────────────────────────┘
```
As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.
ACKs for top commit:
jonatack:
ACK e3a06a3c6c
Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25
027aab663a test, contrib, refactor: use `with` when opening a file (brunoerg)
Pull request description:
When manipulating a file in Python without using `with()`, you have to close the file manually, so this PR does it in `get_block_hashes` (`contrib/linearize/linearize-data.py`).
Edit: this PR does it for all occurances that previously weren't using `with`.
ACKs for top commit:
laanwj:
Code review ACK 027aab663a
Tree-SHA512: 879400968e0013e8678ec16f1fe5d0963a73c1e0d442ca34802d885214f0783d2e9a9b500fc6be7c3b93560a367b6a3d685eee24d2f9ce53fddf064ea6feecf8
f849e63bad fuzz: SplitString with multiple separators (Martin Leitner-Ankerl)
d1a9850102 http: replace boost::split with SplitString (Martin Leitner-Ankerl)
0d7efcdf75 core_read: Replace boost::split with SplitString (Martin Leitner-Ankerl)
b7ab9db545 Extend Split to work with multiple separators (Martin Leitner-Ankerl)
Pull request description:
As a followup of #22953, this removes the remaining occurrences of `boost::split` and replaces them with our own `SplitString`. To be able to do so, this extends the function `spanparsing::Split` to work with multiple separators. Finally this removes 3 more files from `lint-includes.py`.
ACKs for top commit:
theStack:
Code-review ACK f849e63bad
Tree-SHA512: f37d4dbe11cab2046e646045b0f018a75f978d521443a2c5001512737a1370e22b09247d5db0e5c9e4153229a4e2d66731903c1bba3713711c4cae8cedcc775d
d1bfe5ebdb test: add coverage for invalid requests for `blockfilterheaders` (brunoerg)
Pull request description:
This PR adds test coverage for invalid requests (`Invalid hash` and `Unknown filtertype`) for `/blockfilterheaders` in REST functional test.
ACKs for top commit:
jonatack:
ACK d1bfe5ebdb
vincenzopalazzo:
ACK d1bfe5ebdb
Tree-SHA512: 9ab7efe7131296577c60642f95921799cf1dbae9c2aaea6752d2ac9f35a1bcc72b9d742a146c314f82fe1848190a80c88836ab78fc28773ed12e97fa327828e7
Note that `SplitString` doesn't support token compression, but in this case
it does not matter as empty strings are already skipped anyways.
Also removes split.hpp and classification.hpp from expected includes
a498acce45 test: MiniWallet: skip mempool check if `mempool_valid=False` (Sebastian Falbesoner)
01552e8f67 test: MiniWallet: always rehash after signing (P2PK mode) (Sebastian Falbesoner)
Pull request description:
MiniWallet's core method for creating txs (`create_self_transfer`) right now always executes the `testmempoolaccept` RPC to check for mempool validity or invalidity. In some test cases where we use MiniWallet to create a huge number of transactions this can lead to performance issues, in particular feature_fee_estimation.py where the execution time after MiniWallet usage (PR #24817) doubled, see https://github.com/bitcoin/bitcoin/issues/24828#issuecomment-1100058100, https://github.com/bitcoin/bitcoin/issues/24828#issuecomment-1100301980. This PR mitigates this by skipping the mempool check if the parameter `mempool_valid` is set to `False`.
As a preparatory commit, the test feature_csv_activation.py has to be adapted w.r.t. to rehashing of transactions, as we now hash all transactions immediately in `create_self_transfer` in order to get the txid (before we relied on the result of `testmempoolaccept`).
On my machine, this decreases the execution time quite noticably:
master branch:
```
$ time ./test/functional/feature_fee_estimation.py
real 3m20.771s
user 2m52.360s
sys 0m39.340s
```
PR branch:
```
$ time ./test/functional/feature_fee_estimation.py
real 2m1.386s
user 1m42.510s
sys 0m22.980s
```
Partly fixes#24828 (hopefully).
ACKs for top commit:
danielabrozzoni:
tACK a498acce45
Tree-SHA512: f20c358ba42b2ded86175f46ff3ff9eaefb84175cbd1c2624f44904c8d8888e67ce64d6dcbb26aabbf07906e6f5bdea40353eba9ae668618cadcfc517ef7201b
fad0abf539 lint: Fix lint-circular-dependencies.py file list (MacroFake)
Pull request description:
currently in-tree files like `wallet/test/fuzz/coinselection.cpp` are missed. Also out-of-tree files like `test/data/bip341_wallet_vectors.json.h` or `qt/moc_qvaluecombobox.cpp` are included.
Change the script to only use in-tree files.
Also, change `'python3'` to `sys.executable`.
ACKs for top commit:
laanwj:
Code review ACK fad0abf539
Tree-SHA512: baf150fbae6a7120b2692f2eaef6a7773f2681e1610f8776f8d2ae6736c74736502a505df080b2182880f753b90f94e76a1e365fb45185f46f0e4d5521ca8e86
5f213213cb tests: add tests for cross-chain wallet use prevention (Seibart Nedor)
968765973b wallet: ensure wallet files are not reused across chains (Seibart Nedor)
Pull request description:
This implements a proposal in #12805 and is a rebase of #14533.
This seems to be a working approach, but I'm not sure why the `p2p_segwit.py` functional test needed a change, so I'll look into it more.
ACKs for top commit:
achow101:
ACK 5f213213cb
dongcarl:
Code Review ACK 5f213213cb
[deleted]:
tACK 5f213213cb
Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec
fafd67479a test: Remove previous release check (MarcoFalke)
Pull request description:
Now that the commit (7c08d81e11) which changes taproot to be enforced for all blocks is sufficiently buried by other commits, and thus less likely to be reverted, it seems a good time to remove no longer needed test code.
The `feature_taproot` functional test is cleaned up to no longer run against a previous release. Since previous releases are static and impossible to change, it is sufficient to run the test once against the release. Now that this is done, the check can be removed without decreasing test coverage.
ACKs for top commit:
laanwj:
Concept and code review ACK fafd67479a
vincenzopalazzo:
ACK fafd67479a
Tree-SHA512: fcb1a93f3bf9deb5f5c7327a7cd23be10ba09c9f4cbfa73ee2764a93c6ce7d6fa98ca32f2cf4023c20ab624aee601beec949fd02a57a3a658fdbd4be1a9ff338
fa82a1ed83 lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake)
Pull request description:
Follow up to commit b1c5991eeb. Also remove empty newline added in that commit.
ACKs for top commit:
fanquake:
ACK fa82a1ed83
Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478
29f44fed36 Converting `lint-all.sh` to `lint-all.py`. (hiago)
Pull request description:
This PR is converting `test/lint/lint-all.sh` to `test/lint/lint-assertions.py`. It's an item of https://github.com/bitcoin/bitcoin/issues/24783.
ACKs for top commit:
laanwj:
Tested ACK 29f44fed36
Tree-SHA512: 5427936aaa8e009613048448cbd79d9225675bdcadcf4fbb70fd091e0aab85e350ef1678da1b388f70d62ca16f40409409f90da3f6bbf057480522cd50fde811
Add `strerror` to the locale-dependence linter to catch its use. Add
exemptions for bdb interface code (false positive) and strerror.cpp
(the only allowed use).
Also fix a bug in the regexp so that `_r` and `_s` variants are detected
again.
786b3a7c44 tests: Do not always create a descriptor wallet in wallet_createwallet (Andrew Chow)
Pull request description:
The createwallet test for some invalid parameters incorrectly always creates a descriptor wallet. This is unnecessary and also breaks the test when bdb is not compiled in.
Fixes#25007
ACKs for top commit:
jacobpfickes:
ACK 786b3a7c44
Tree-SHA512: 97b0953a08adf83d5ea84cac2651253d790b43d606a2f746dd45d3ccd1fb576bab63e3835e3de592715ef8a5cb133e6f19a3ab810fedf4684072143c3cb578d4
The createwallet teswt for some invalid parameters incorrectly always
creates a descriptor wallet. This is unnecessary and also breaks the
test when bdb is not compiled in.
2ff8f4dd81 Add tests for addr destination rotation (Gleb Naumenko)
77ccb7fce1 Use std::chrono for salting when randomizing ADDR destination (Gleb Naumenko)
Pull request description:
We currently assign a destination peer for relaying particular addresses of nodes every 24 hours, and then rotate. This is done for rate-limiting (ultimately for privacy leak reduction I think?).
Before this change, 24 hours was defined as uint. I replaced it with std::chrono, which is mockable and type-safe.
Also added couple tests for this behavior.
ACKs for top commit:
jonatack:
ACK 2ff8f4dd81
Tree-SHA512: 16f703ef3ffee13ce3afa82ca7b4baa27308af18cd2eececdce5565badfb68656a2ad9c4594b73772e4bfa99b3fb15f8e4089c1cb4be98c0bae6730a9d2f8a25
fa1f6df21e test: Fix intermittent test failure in wallet_listreceivedby.py (MarcoFalke)
Pull request description:
* Remove not needed "Generate block to get out of IBD"
* Sync blocks where possible to avoid incoming blocks on the p2p `msghand` thread while blocks are mined in the RPC thread. See https://github.com/bitcoin/bitcoin/issues/24730 for discussion.
Top commit has no ACKs.
Tree-SHA512: eca0242e7793886535555fec62f7acd4c0955bf26fab78725b4fe53f84f0b118cb12c9ee35627503fc68b83c3a228842e861fab89aab1226e08e18596357aaae
71c3f0356c move-only: Rename index + pruning functional test (Fabian Jahr)
de08932efa test: Update test for indices on pruned nodes (Fabian Jahr)
825d19839b Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb0c6 Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823531 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b035 refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)
Pull request description:
# Motivation
The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).
# Background
`coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.
Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.
# Alternative approach
Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
- Usage of globals
- Blocks pruning with a start and a stop height
- Can persist blockers across restarts
- Blockers can be set/unset via RPCs
Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.
ACKs for top commit:
mzumsande:
Code review ACK 71c3f0356c
ryanofsky:
Code review ACK 71c3f0356c. Changes since last review: just tweaking comments and asserts, and rebasing
w0xlt:
tACK 71c3f0356c on signet.
Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
ab5af9ca72 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0 wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181 wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca72
josibake:
crACK ab5af9ca72
0xB10C:
ACK ab5af9ca72. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>`
error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or
`settings.json` value is specified, and just makes bitcoin connect and listen
normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003
to prevent a bare `-proxy` command line argument with no assignment from
clearing proxy settings. But it was implemented in an overbroad way breaking
empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with
https://github.com/bitcoin/bitcoin/pull/15936, reported in
https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by
vasild, that happens after a proxy setting is enabled and disabled in the GUI.
But this change also makes sense on its own to remove a potentially confusing
error message.
dac44fc06f init: disallow reindex-chainstate with optional indexes (Martin Zumsande)
62e14285f9 doc: Add note that -reindex will rebuild optional indexes (Martin Zumsande)
Pull request description:
When started together with `-reindex-chainstate`, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens.
This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an `InitError` when they are activated, thus requiring the user to deactivate them temporarily until the `-reindex-chainstate` run is finished.
This also disallows `-reindex-chainstate` in combination with `-txindex`, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly.
As a long-term goal, it would be desirable to have the indexes tolerate `reindex-chainstate` by ignoring their `BlockConnected` notifications (there is discussion in #24630 about this) or possibly move `reindex-chainstate` option into a `bitcoin-chainstate` executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes.
The first commit adjusts the `-reindex` doc to mention that this option does rebuild all active indexes.
ACKs for top commit:
ryanofsky:
Code review ACK dac44fc06f. Just fixed IsArgSet call and edited error messages since last review
Tree-SHA512: c1abf7d350648ae227c3fd6c95d9a54c3bac9de70915275dea1c87cca6d9a76a056c0e306d95ef8cfe4df1f8525b418e0e7a4f52ded3be464041c0dc297f8930
