fada6c65d2 wallet: Strictly match tx change type to improve privacy (MarcoFalke)
Pull request description:
Currently the change type will only match a destination by accident, making it easier to determine the change.
Fix that by strictly matching one of the destinations.
ACKs for top commit:
S3RK:
Concept & Approach ACK fada6c6. Also did light code review .
achow101:
ACK fada6c65d2
prayank23:
tACK fada6c65d2
w0xlt:
tACK fada6c6
Tree-SHA512: 2b072c3c32debac7b0bef07a6df9a8f1a631e0f7d556b859973f18894ca490225582dc13e4588b29fa205ffbcd30fb632d5313b304d10ad17a26adc3f7684471
140a49ce5e test: check that pruneblockchain RPC fails for future block or timestamp (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `pruneblockchain` RPC for the case that a future block or timestamp is passed:
8c0bd871fc/src/rpc/blockchain.cpp (L1101)8c0bd871fc/src/rpc/blockchain.cpp (L1111)
Note that the test method `manual_test` gets called twice, once each with `use_timestamp` set to True/False, respectively. Depending on that, the helper function `height` either converts the passed block height to the timestamp of that block, or just returns it without modification.
The other tests for failures in this RPC are also changed to be more detailled ("Cannot prune blocks because node is not in prune mode", "Negative block height"), as I don't think there is any value in just checking a sub-string. If there is ever an error with the same sub-string is introduced, it's not clear which error is exactly checked with the test, so it makes sense to be as specific as possible.
ACKs for top commit:
brunoerg:
tACK 140a49ce5e
Tree-SHA512: bee3cee9f35c2a63a1839d7ec1f83e354d9d3c0c2ca32d300dca2de8b755d555f769ba2b80ac37d31df6ee7e2b8eaefb8134c4727a7144e47c0f5e34f2cc5822
0a1b6fa5a1 test: fix intermittent timeouts in p2p_timeouts.py (Martin Zumsande)
Pull request description:
Fixes #23800 by making sure that all peers are connected (i.e. `m_connected` is set) before the mocktime is bumped.
We can't wait for verack here, but we can wait for a debug log entry ("Added connection peer=2") instead.
In the failed CI runs (e.g. https://cirrus-ci.com/task/5600553806856192?logs=ci#L7469) different peers were added at different mocktimes.
ACKs for top commit:
naumenkogs:
ACK 0a1b6fa5a1
theStack:
Concept and approach ACK 0a1b6fa5a1
Tree-SHA512: 1a3c8a9a79339d4adc6ecb1731eb0d0eadb2e5024ad3c6779b4696691f85d6c3304ef8689746d0332150a4cf04489ca4b2ff3eeb0bb76feec28c1e4bb9dbca19
2b64fa3251 Update REST docs with new accessors (Matt Corallo)
ef7c8228fd Expose block filters over REST. (Matt Corallo)
Pull request description:
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
You can test it at 000000005b.hex
ACKs for top commit:
dergoegge:
ACK 2b64fa3251 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.
Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696
261dddb924 test: Combine addr generation helper functions (Martin Zumsande)
aeeccd9aa6 test: Fix intermittent issue in p2p_addr_relay.py (Martin Zumsande)
Pull request description:
Fixes#22449 by increasing the mocktime jump (just as in 6168eb06b2), which prevents failures due to rare Poisson timer events, or at least makes them a lot more unlikely.
The second commit combines the addr generation helper functions `setup_addr_msg` and `setup_rand_addr_msg`. It also changes the way `addr.time` is filled to random, because before, if too many addresses (>600) were created in a batch, they would stop being relayed because their timestamp would be too far in the future.
ACKs for top commit:
josibake:
reACK 261dddb924
jnewbery:
utACK 261dddb924
Tree-SHA512: d0eca887de4bc85092730284cf612193d2c12b0a3d624a2bfa5fef4a5890d3b6375c564333c5927425958e4b6ec86b8854b18b2233c7b6f1691d9ddc397948a9
This combines the addr generation helper functions setup_addr_msg
and setup_rand_addr_msg.
It also changes the way addr.time is filled to random, because before,
if too many addresses (>600) were created in a batch, they would stop
being relayed because their timestamp would be too far in the future.
62fa61fa4a refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48 refactor: Implement restorewallet() logic in the wallet section (w0xlt)
Pull request description:
Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it.
This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in https://github.com/bitcoin-core/gui/pull/471 ).
This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed.
ACKs for top commit:
achow101:
ACK 62fa61fa4a
shaavan:
crACK 62fa61fa4a
Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39
618f4d2890 test: re-organized array according to order of logs and included 2 more interruption events (seaona)
71115a5e23 test: include two more interruptions points (seaona)
Pull request description:
This PR aims to introduce 2 more interruption points in the process of initialization, in order to make the` feature_init `testcase more complete. These are the following:
-` Checking all blk files are present`
-` init message: Starting network threads`
It is a small improvement for increasing the coverage of potential interruptions, and making sure that the node can restart successfully after these interruptions.
ACKs for top commit:
jamesob:
ACK 618f4d2890, pending CI
jarolrod:
ACK 618f4d2890
Tree-SHA512: 9d709734e298e955709094bb97478ca7f18859874f1ba026f7c9014d87205aea63f6cf2093ebee600eaf82d3245adb11e77fae24a1ae48b69efefd57f3def921
The -sandbox argument is not present in the v22.0 release. Changing the minimum version to 229900 ensures it's used when testing the master branch.
If the argument is backported, the minimum version can be adjusted to e.g. 220100.
This cleanup may slightly impact test coverage.
Reduce code repition by looping over the list of nodes.
Reduce brittleness by refering to nodes by name rather than index.
Add helper method nodes_wallet_dir.
fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
eaf6be0114 [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b1 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114
MarcoFalke:
review ACK eaf6be0114🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
fa24a3df87 rpc: Quote user supplied strings in error messages (MarcoFalke)
Pull request description:
I can't see a downside doing this and this fixes a fuzzing crash
Background:
This is a follow-up to commit 926fc2a0d4, which introduced the "starts_with-hack". Maybe an alternative to the hack would be to assign a unique error code to internal bugs? However, I think this can be done in an separate pull request and the changes here make sense even on their own.
ACKs for top commit:
fanquake:
ACK fa24a3df87 - to fix the fuzzers.
Tree-SHA512: d998626406a64396a037a6d1fce22fce3dadb7567c2f9638e450ebe8fb8ae77d134e15dd02555326732208f698d77b0028bc62be9ceee9c43282b61fe95fccbd
41b9f7d062 test: Use byte unit 'M' for -maxuploadtarget functional test (Douglas Chimento)
Pull request description:
ACKs for top commit:
shaavan:
ACK 41b9f7d062
stratospher:
ACK 41b9f7d.
Tree-SHA512: 25b46347c671e8d6fd8878e7fee40e773bb03641e53e41e8a79a286fe4a0cf71c0c0986d6d7418fcb656c07f7216cc50a7ee4366f9213c32b01ae74326031f80
9600ea0145 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524698 index: Fix backwards search for bestblock (Martin Zumsande)
Pull request description:
This PR attempts to fix an intermittent Init issue encountered during the stress testing of #23289, which relates to the pruning-compatible filter reconstruction logic introduced in #15946.
The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
In that case, during the next restart of the node, an Init error would be thrown because `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
The loop
`while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.
To reproduce this bug on regtest:
1) start a node with a fresh datadir using `-txindex=1` (or any other index)
2) stop and restart without any index
3) mine a block
3) stop and restart again with the index enabled
->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`
Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.
ACKs for top commit:
ryanofsky:
Partial code review ACK 9600ea0145 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.
Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38
d5cab1a96d Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson)
e46fc935aa Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson)
d1a9742623 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson)
Pull request description:
Fixes#21368
Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different.
ACKs for top commit:
achow101:
ACK d5cab1a96d
Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
c0405ee27f rpc: Document that DEFAULT is for Taproot, ALL for everything else (Andrew Chow)
d3992669df psbt: Actually use SIGHASH_DEFAULT (Andrew Chow)
eb9a1a2c59 psbt: Make sighash_type std::optional<int> (Andrew Chow)
Pull request description:
Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs.
ACKs for top commit:
Sjors:
re-utACK c0405ee27f
Tree-SHA512: 5199fb41de416b2f10ac451f824e7c94b428ba11fdb9e50f0027c692e959ce5813a340c34a4e52d7aa128e12008303d80939a693eff36a869720e45442119828
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836Fixes#20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
5493e92501 Check descriptors returned by external signers (sstone)
Pull request description:
Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
See https://github.com/bitcoin/bitcoin/issues/23627 for context.
The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`.
I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`.
ACKs for top commit:
jamesob:
Code review ACK 5493e92501
achow101:
ACK 5493e92501
Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
This simplifies the code, and slightly speeds up the test.
Running `./test/functional/test_runner.py -j15 $(printf 'feature_fee_estimation %.0s' {1..15})`
on master 3 times gives:
- Before:
ALL | ✓ Passed | 788 s (accumulated)
ALL | ✓ Passed | 818 s (accumulated)
ALL | ✓ Passed | 873 s (accumulated)
- After:
ALL | ✓ Passed | 763 s (accumulated)
ALL | ✓ Passed | 798 s (accumulated)
ALL | ✓ Passed | 731 s (accumulated)
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
84bc35d7a5 test: feature_rbf.py: check specified wallet type availability (Sebastian Falbesoner)
Pull request description:
The test currently leads to a failure if in general wallet support is compiled, but the library for the specified type (BDB/SQLite) is not, i.e. if started with the `--legacy-wallet` parameter, but bitcoind is compiled without BDB support, see e.g. https://github.com/bitcoin/bitcoin/pull/23682#issuecomment-989044207
Fix this by checking if the specified wallet type (BDB for legacy wallet, SQLite for descriptor wallet) is available.
Also move the helper `is_specified_wallet_compiled()` to the
test framework's class BitcoinTestFramework first, so it can be reused.
Should further pave the way for #23682. On my local instance without BDB compiled, all targets in test_runner pass now.
ACKs for top commit:
mzumsande:
ACK 84bc35d7a5, changes loook good for me and I confirmed that this fixes the error encountered on master when running the test `--without-bdb`.
Tree-SHA512: 1575c03c793c8e0ac195d0914eff75d02604301c8fb77d0fdb7c0b245561569c0c7db387ef4de499044b68ab6e14b4b78b955f6e74c84197bcaed95f439c9824
5b559dc7ec Swap out hashlib.ripemd160 for own implementation (Pieter Wuille)
ad3e9e1f21 Add pure Python RIPEMD-160 (Pieter Wuille)
Pull request description:
Closes#23710.
ACKs for top commit:
jamesob:
ACK 5b559dc7ec, pending CI
Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
The test currently leads to a failure if in general wallet
support is compiled, but the library for the specified type
(BDB/SQLite) is not, i.e. if started with the
`--legacy-wallet` parameter, but bitcoind is compiled
without BDB support.
Fix this by checking if the specified wallet type (BDB for
legacy wallet, SQLite for descriptor wallet) is available.
Also move the helper `is_specified_wallet_compiled()` to the
test framework's class BitcoinTestFramework first, so it can
be reused.
Currently the test performs the wallet-relevant parts if
_any_ wallet type support is compiled in, independently of
whether the test is run with legacy or descriptor wallet
specified. This leads to a failure if the test is started
with the `--legacy-wallet` parameter, but bitcoind is
compiled without BDB support.
Fix this by checking if the specified wallet type (BDB for
legacy wallet, SQLite for descriptor wallet) is available.
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
1dcba996d3 Coinbase receivedby rpcs release notes (Andrew Toth)
b5696750a9 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth)
bce20c34d6 Include coinbase transactions in receivedby wallet rpcs (Andrew Toth)
Pull request description:
The current `*receivedby*` RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction *is* receiving those coins.
This PR corrects this behaviour. Also, a new option `include_immature_coinbase` is added (default=`false`) that includes immature coinbase transactions when set to true.
However, since this is potentially a breaking change this PR introduces a hidden configuration option `-deprecatedrpc=exclude_coinbase`. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release.
Fixes https://github.com/bitcoin/bitcoin/issues/14654.
ACKs for top commit:
jnewbery:
reACK 1dcba996d3
Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df
2f9515f37a rpc: move fees object to match help (josibake)
07ade7db8f doc: add release note for fee field deprecation (josibake)
2ee406ce3e test: add functional test for deprecatedrpc=fees (josibake)
35d928c632 rpc: deprecate fee fields from mempool entries (josibake)
Pull request description:
per #22682 , top level fee fields for mempool entries have been deprecated since 0.17 but are still returned. this PR properly deprecates them so that they are no longer returned unless `-deprecatedrpc=fees` is passed.
the first commit takes care of deprecation and also updates `test/functional/mempool_packages.py` to only use the `fees` object. the second commit adds a new functional test for `-deprecatedrpc=fees`
closes#22682
## questions for the reviewer
* `-deprecatedrpc=fees` made the most sense to me, but happy to change if there is a name that makes more sense
* #22682 seems to indicate that after some period of time, the fields will be removed all together. if we have a rough idea of when this will be, i can add a `TODO: fully remove in vXX` comment to `entryToJSON`
## testing
to get started on testing, compile, run the tests, and start your node with the deprecated rpcs flag:
```bash
./src/bitcoind -daemon -deprecatedrpc=fees
```
you should see entries with the deprecated fields like so:
```json
{
"<txid>": {
"fees": {
"base": 0.00000671,
"modified": 0.00000671,
"ancestor": 0.00000671,
"descendant": 0.00000671
},
"fee": 0.00000671,
"modifiedfee": 0.00000671,
"descendantfees": 671,
"ancestorfees": 671,
"vsize": 144,
"weight": 573,
...
},
```
you can also check `getmempoolentry` using any of the txid's from the output above.
next start the node without the deprecated flag, repeat the commands from above and verify that the deprecated fields are no longer present at the top level, but present in the "fees" object
ACKs for top commit:
jnewbery:
reACK 2f9515f37a
glozow:
utACK 2f9515f37a
Tree-SHA512: b175f4d39d26d96dc5bae26717d3ccfa5842d98ab402065880bfdcf4921b14ca692a8919fe4e9969acbb5c4d6e6d07dd6462a7e0a0a7342556279b381e1a004e
fa5362a9a0 rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs (MarcoFalke)
Pull request description:
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
ACKs for top commit:
meshcollider:
utACK fa5362a9a0
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
0ba98eda28 test: remove unneeded sync_all() calls in wallet_listtransactions.py (Sebastian Falbesoner)
Pull request description:
This is a small follow-up to #23659. The `self.sync_all()` calls after generating blocks can be removed, since that happens automatically per default by the test framework's generate function (if no explicit sync_fun is passed).
On the course of touching the file, imports are sorted and the grammar of a log message is fixed.
ACKs for top commit:
fanquake:
ACK 0ba98eda28 - thanks for following up.
shaavan:
ACK 0ba98eda28
Tree-SHA512: 451e733865dcb1e424d90289c8c89272837a9af6fd4b77d6c60728c84524d9c792d684b7e601b02a0efda67231183c42dd9040d96214ac7d9473b2808cabe73f
99993425af rpc: Only allow specific types to be P2(W)SH wrapped in decodescript (MarcoFalke)
Pull request description:
It seems confusing to return a P2SH wrapping address that is eventually either policy- or consensus-unspendable.
ACKs for top commit:
laanwj:
Code review re-ACK 99993425af
Tree-SHA512: 3cd530442acee7c295d244995f0f17b2cae7212f1e0970bb5807621f8ff8e4308a3236b385d77087cd493d32ee524813d8edd15e91d937ef9a800094b7bc4946
e4a54af6b8 test: add wallet_transactiontime_rescan.py --descriptors to test_runner.py (Sebastian Falbesoner)
b60e02e993 test: fix test wallet_transactiontime_rescan.py for descriptor wallets (Sebastian Falbesoner)
a905ed1a61 test: refactor: use `set_node_times` helper in wallet_transactiontime_rescan.py (Sebastian Falbesoner)
Pull request description:
The functional test wallet_transactiontime_rescan.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, the test framework maps the importaddress RPC calls to the importdescriptors RPC (rescan=False -> timestamp='now'), which always rescans blocks of the past 2 hours, based on the current MTP timestamp. In order to avoid importing the last address (wo3), we generate 10 more blocks with advanced time, to ensure that the balance after importing is zero:
681b25e3cd/test/functional/wallet_transactiontime_rescan.py (L125-L134)
Calling this test with descriptor wallets is also added to test runner. Fixes#23562.
ACKs for top commit:
Sjors:
tACK e4a54af
brunoerg:
tACK e4a54af6b8
Tree-SHA512: 9fd8e298d48dd7947b1218d61a1a66c1241b3dbb14451b0ec7cd30caa74ee540e7ee5a7bd10d421b9e3b6e549fa5c3e85bd02496436128b433b328118642f600
a4fe70171b Make Bech32 LocateErrors return error list rather than using out-arg (Samuel Dobson)
2fa4fd1961 Use std::iota instead of manually pushing range (Samuel Dobson)
405c96fc9f Use bounds-checked array lookups in Bech32 error detection code (Samuel Dobson)
28d9c2857f Simplify encoding of e in GF(1024) tables to (1,0) (Samuel Dobson)
14358a029d Replace GF1024 tables and syndrome constants with compile-time generated constexprs. (Samuel Dobson)
63f7b69779 Update release note for bech32 error detection (Samuel Dobson)
c8b9a224e7 Report encoding type in bech32 error message (Samuel Dobson)
92f0cafdca Improve Bech32 boost tests (Samuel Dobson)
bb4d3e9b97 Address review comments for Bech32 error validation (Samuel Dobson)
Pull request description:
A number of follow-ups and improvements to the bech32 error location code, introduced in #16807.
Notably, this removes the hardcoded GF1024 tables in favour of constexpr table generation.
ACKs for top commit:
laanwj:
Re-ACK a4fe70171b
Tree-SHA512: 6312373c20ebd6636f5797304876fa0d70fa777de2f6c507245f51a652b3d1224ebc55b236c9e11e6956c1e88e65faadab51d53587078efccb451455aa2e2276
b35942e500 tests: Add data-driven testcases to rpc_decodescript.py (Dimitri)
Pull request description:
closes#23641
Top commit has no ACKs.
Tree-SHA512: 2f494c78ad085d523fae15befaadb9e0fc382b5310e3a95395ecf06a90968b15b6d232f7564098ed0a68419e27aa2e5260fe691cf2ce84af9fb6b65634e54d77
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
3d71d16d1e test: listtranscations with externally generated addresses (S3RK)
d04566415e Add to spends only transcations from me (S3RK)
9f3a622b1c Automatically add labels to detected receiving addresses (S3RK)
c1b99c088c Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses (S3RK)
03840c2064 Add CWallet::IsInternalScriptPubKeyMan (S3RK)
456e350926 wallet: resolve ambiguity of two ScriptPubKey managers providing same script (S3RK)
Pull request description:
This PR fixes certain use-cases when **send-to-self** transactions are missing from `listtransactions` output.
1. When a receiving address is generated externally to the wallet
(e.g. same wallet running on two nodes, or by 3rd party from xpub)
2. When restoring backup with lost metadata, but keypool gap is not exceeded yet
When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label.
Works both for legacy and descriptors wallets.
- For legacy it uses the internal flag from the keypool entry. Caveat: because we don't know which script type would be used we add all possible destinations for such keys.
- For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors.
fixes#19856fixes#20293
ACKs for top commit:
laanwj:
Code review ACK 3d71d16d1e
Tree-SHA512: 03fafd5548ead0c4ffe9ebcc9eb2849f1d2fa7270fda4166419b86877d4e57dcf04460e465fbb9c90b42031f3c05d1b83f1b67a9f82c2a42980825ed1e7b52e6
fa551b3bdd Remove GetAdjustedTime from init.cpp (MarcoFalke)
fa815f8473 Replace addrman.h include with forward decl in net.h (MarcoFalke)
Pull request description:
It seems confusing to call `GetAdjustedTime` there, because no offset could have been retrieved from the network at this point. Even if connman was started, `timedata` needs at least 5 peer connections to calculate an offset.
Fix the confusion by replacing `GetAdjustedTime` with `GetTime`, which does not change behavior.
Also:
* Replace magic number with `MAX_FUTURE_BLOCK_TIME` to clarify the context
* Add test, which passes both on current master and this pull request
* An unrelated refactoring commit, happy to drop
ACKs for top commit:
dongcarl:
Code Review ACK fa551b3bdd, noticed the exact same thing here: e073634c37
mzumsande:
Code Review ACK fa551b3bdd
jnewbery:
Code review ACK fa551b3bdd
shaavan:
ACK fa551b3bdd
theStack:
Code-review ACK fa551b3bdd
Tree-SHA512: 15807a0e943e3e8d8c5250c8f6d7b56afb26002b1e290bf93636a2c747f27e78f01f1de04ce1a83d6339e27284c69c43e077a8467545c4078746f4c1ecb1164d
ffd09281fe rpc: various fixups for dumptxoutset (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
A few fixes to make this RPC actually useful when generating snapshots.
- Generate an assumeutxo hash and display it (sort of a bugfix)
- Add nchaintx to output (necessary for use in chainparams entry)
- Add path of serialized UTXO file to output
ACKs for top commit:
laanwj:
Code review ACK ffd09281fe
Tree-SHA512: b0b5fd5138dea0e21258b1b18ab75bf3fd1628522cc1dbafa81af9cb9fa96562a1c39124fdb31057f256bfc560f462f907e9fe5e209b577b3f57afae2b7be826
d9803f7a0a test: add stress tests for initialization (James O'Beirne)
23f85616a8 test: add node.chain_path and node.debug_log_path (James O'Beirne)
Pull request description:
In the course of coming up with a test plan for #23280, I thought it would be neat to include a Python snippet showing how I tested the initialization process. I quickly realized I was reinventing the functional test framework... so here's a new test.
This change bangs init around like the Fonz hitting a jukebox. It adds some interesting (read: lazy and random) coverage for the initialization process by
- interrupting init with SIGTERM after certain log statements,
- interrupting init at random points, and
- starting init with some essential data missing (block files, block indices, etc.) to test init error paths.
As far as I can tell, some of these code paths are uncovered otherwise (namely the startup errors).
---
Incidentally, I think I may have uncovered some kind of a bug or race condition with indexing initialization based on an intermittent failure in this testcase. This test sometimes fails after shutting down immediately after `loadblk` thread start:
```
2021-10-15T21:14:51.295000Z TestFramework (INFO): Starting node and will exit after line 'loadblk thread start'
36 │ 2021-10-15T21:14:51.296000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
37 │ 2021-10-15T21:14:51.493000Z TestFramework (INFO): terminating node after 110 log lines seen
38 │ 2021-10-15T21:14:51.625000Z TestFramework (INFO): Starting node and will exit after line 'txindex thread start'
39 │ 2021-10-15T21:14:51.625000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
------> [[ FAILURE HERE ]] 2021-10-15T21:15:21.626000Z TestFramework (WARNING): missed line {bail_line}; bailing now after {num_lines} lines
```
and then fails to start up afterwards. Combined logs showing `Error: txindex best block of the index goes beyond pruned data`, when the node under test is not pruned:
```
node0 2021-10-15T21:16:51.848439Z [shutoff] [validationinterface.cpp:244] [ChainStateFlushed] Enqueuing ChainStateFlushed: block hash=1014bc4ff4917602ae53d10e9dfe230af4b7d52a6cdaa8a47798b9c288180907
node0 2021-10-15T21:16:51.848954Z [shutoff] [init.cpp:302] [Shutdown] Shutdown: done
test 2021-10-15T21:16:51.882000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 87, in run_test
check_clean_start()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 60, in check_clean_start
node.wait_for_rpc_connection()
File "/home/james/src/bitcoin/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
test 2021-10-15T21:16:51.882000Z TestFramework (DEBUG): Closing down network thread
test 2021-10-15T21:16:51.933000Z TestFramework (INFO): Stopping nodes
test 2021-10-15T21:16:51.933000Z TestFramework.node0 (DEBUG): Stopping node
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
```
ACKs for top commit:
laanwj:
Code review ACK d9803f7a0a
Tree-SHA512: 4d80dc399daf199a6222e81e47d12d830dc7af07355eddbb7f52479a676a645b8d3d45093ff54a9295f01a163b2f4fe0e038e83fc269969e03d4cfda69eaf111
6aed8b7e9b [test] tx processing before and after ibd (glozow)
b9e105b664 [net_processing] ignore all transactions during ibd (glozow)
Pull request description:
This is basically a mini, IBD-only version of #21224
Incoming transactions aren't really relevant until we're caught up. That's why we send a giant feefilter and don't send tx getdatas, but we also shouldn't process them if peers send them anyway. Simply ignore them.
ACKs for top commit:
jnewbery:
reACK 6aed8b7e9b
laanwj:
Code review ACK 6aed8b7e9b
Tree-SHA512: 8e1616bf355f9d0b180bdbc5461f24c757dc5d7bc7bf651470f3b0bffcca5d5e68287106255b5cede2d96b42bce448a0f8c0649de35a530c5e079f7c89c70a35
- Actually generate an assumeutxo hash and display it
- Add nchaintx to output (necessary for use in chainparams entry)
- Add path of serialized UTXO file to output
a99ed89865 psbt: sign without finalizing (Andrew Chow)
Pull request description:
It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default.
This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast.
ACKs for top commit:
meshcollider:
utACK a99ed89865
Sjors:
utACK a99ed89
Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6
988024fe37 test: add check_node_connections in util (brunoerg)
Pull request description:
This function (`check_node_connections`) exists in `feature_anchors.py` and `p2p_add_connections.py` and does the same thing in both, this PR moves this function to util to avoid repetition and facilitate maintenance.
ACKs for top commit:
laanwj:
Code review ACK 988024fe37
Tree-SHA512: bf86c5659933539c72cb91ad587552b45c918be74d36fb429e78f3b954f01ed0855a85dd49aea35b432fbd18227c05eb3fec8b99c139c3509c39b19bccf6b7fd
fa3e0da06b policy: Treat taproot as always active (MarcoFalke)
Pull request description:
Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things:
* Importing `tr` descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate `tr` descriptors by default (regardless of the taproot status) after commit 47fe7445e7.
* Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe7445e7, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See https://github.com/bitcoin/bitcoin/issues/11887.
A similar change was done for segwit v0 in https://github.com/bitcoin/bitcoin/pull/13120 .
This effectively reverts commit c5ec0367d7.
ACKs for top commit:
mjdietzx:
Code Review ACK fa3e0da06b
achow101:
ACK fa3e0da06b
sipa:
utACK fa3e0da06b
gruve-p:
ACK fa3e0da06b
gunar:
Code Review + tACK fa3e0da06
rajarshimaitra:
code review + tACK fa3e0da06b
Tree-SHA512: c6dc7a4e6c345bdec33f256847dc63906ab1696aa683ab9b32a79e715613950884ac3a1a7a44e95f31bb28e58dd64679a616175f7e152b21f5550f3337c8e622
88cc481092 Modify copyright header on Bech32 code (Samuel Dobson)
5599813b80 Add lots of comments to Bech32 (Samuel Dobson)
2eb5792ec7 Add release notes for validateaddress Bech32 error detection (MeshCollider)
42d6a029e5 Refactor and add more tests for validateaddress (Samuel Dobson)
c4979f77c1 Add boost tests for bech32 error detection (MeshCollider)
02a7bdee42 Add error_locations to validateaddress RPC (Samuel Dobson)
b62b67e06c Add Bech32 error location function (Samuel Dobson)
0b06e720c0 More detailed error checking for base58 addresses (Samuel Dobson)
Pull request description:
Addresses (partially) #16779 - no GUI change in this PR
Adds a LocateError function the bech32 library, which is then called by `validateaddress` RPC, (and then eventually from a GUI tool too, future work). I think modifying validateaddress is nicer than adding a separate RPC for this.
Includes tests.
Based on https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js
Credit to sipa for that code
ACKs for top commit:
laanwj:
Code review and manually tested ACK 88cc481092
ryanofsky:
Code review ACK 88cc481092 with caveat that I only checked the new `LocateErrors` code to try to verify it didn't have unsafe or unexpected operations or loop forever or crash. Did not try to verify behavior corresponds to the spec. In the worst case bugs here should just affect error messages not actual decoding of addresses so this seemed ok.
w0xlt:
tACK 88cc481
Tree-SHA512: 9c7fe9745bc7527f80a30bd4c1e3034e16b96a02cc7f6c268f91bfad08a6965a8064fe44230aa3f87e4fa3c938f662ff4446bc682c83cb48c1a3f95cf4186688
e57c0eb865 test: refactor: replace OP_1/OP_TRUE magic numbers by constants (Sebastian Falbesoner)
df5d783aef test: refactor: take use of `create_block` txlist parameter (Sebastian Falbesoner)
ae9df4ef93 test: refactor: take use of `create_block` version parameter (or use default) (Sebastian Falbesoner)
Pull request description:
The helper `create_block` offers two parameters `version` and `txlist` which set the `nVersion` field / extend the `vtx` array of the block, respectively. By taking use of those, we can remove a lot of code, including the recalculation of the merkle root. Both passing txs in string and `CTransaction` format is supported, i.e. we also save potential calls to `tx_from_hex`.
The PR also contains another commit which replaces magic numbers for OP_TRUE/OP_1 (0x51) with the proper constant from the `script` module.
Instances setting the block version of 4 explicitely after calling `create_block` are removed, as this is the default since #16333 got merged (see https://github.com/bitcoin/bitcoin/pull/23521#discussion_r751173671).
ACKs for top commit:
stratospher:
tested ACK e57c0eb.
Tree-SHA512: a56965168d36b40b84e7f334b00472b82c31e8482c9e2651c97a791abd7fee3b40ca15e943a7acafa3acf172066fdace38bb13240084b789fd6ff4f6e510e23a
4868c9f1b3 Extract Taproot internal keyid with GetKeyFromDestination (Andrew Chow)
d8abbe119c Mention bech32m in -addresstype and -changetype help (Andrew Chow)
8fb57845ee Create a tr() descriptor bech32m DescriptorScriptPubKeyMan by default (Andrew Chow)
54b3699862 Store pubkeys in TRDescriptor::MakeScripts (Andrew Chow)
Pull request description:
Make a `tr()` descriptor by default in descriptor wallets so that users will be able to make and use segwit v1 bech32m addresses.
ACKs for top commit:
MarcoFalke:
Concept ACK 4868c9f1b3
Sjors:
re-utACK 4868c9f1b3
gruve-p:
ACK 4868c9f1b3
meshcollider:
Concept + code review ACK 4868c9f1b3
Tree-SHA512: e5896e665b8d559f1d759b6582d1bb24f70d4698a57307684339d9fdcdac28ae9bc17bc946a7efec9cb35c130a95ffc36e3961a335124ec4535d77b8d00e9631
1e86ff7941 test: run rpc-generateblock.py even with wallet disabled (Darius Parvin)
Pull request description:
Run rpc_generateblock.py test even when the wallet is disabled, as discussed in #20078.
This PR picked up from where #20808 left off, started by @ nginocchio. Since then, there have been many improvements to `MiniWallet`, making this PR more straightforward.
L23 makes use of `MiniWallet.rescan_utxos()` to add the pre-mined block utxos (when `self.setup_clean_chain` is not set to `True`), rather than generating new blocks during the test.
ACKs for top commit:
mjdietzx:
Tested ACK 1e86ff7941
Tree-SHA512: 4285f61516dd53a08004eeea26d58f45b4c1c67f5ca4c94ff1bc9fc7e50f486de2e033a8b4aaf67cb4c33d73aa929362e18dc75d5c7951cbf58120b5fb1de555
Also remove the enumerations ("1)", "2)"...) from the test
cases as those potentially hinder maintainability; e.g. if a
new case in inserted in-between, all the remaining
enumerations would need to be adapted.
Passing a list of transactions `txlist` to `create_block` appends
them to the block, hence we don't need to do that manually anymore.
The merkle root calculation can also be removed, since this is done
in the end of the helper.
f1ed30451f test: refactor: simplify `block_submit` in feature_nulldummy.py (Sebastian Falbesoner)
5ba9f1ff59 test: refactor: rename NULLDUMMY-invalidation helper (Sebastian Falbesoner)
e1d4a128e8 test: simplify and document NULLDUMMY-invalidation helper (Sebastian Falbesoner)
Pull request description:
This PR improves the functional test `feature_nulldummy.py` by simplifying the helpers `trueDummy` (renamed to `invalidate_nulldummy_tx`) and `block_submit`. Details can be found in the commit messages.
ACKs for top commit:
laanwj:
Code review ACK f1ed30451f
Tree-SHA512: ad227b31936f53c5dbded823643bced296d86f40b90f2c144a9857db3d00544f9ad5bbce4c7e84b6ece25e78e95c19aafb1d8fb31e610dcd5cbf3da63190de85
fa9c26ab3a test: Force --nosandbox when --valgrind (MarcoFalke)
Pull request description:
The two options are mutually exclusive and will result in a test failure. Fix that.
Can be tested with:
```
$ ./test/functional/wallet_disable.py --valgrind
ACKs for top commit:
fanquake:
ACK fa9c26ab3a
Tree-SHA512: 7d1c36c1b6627ca041757eb0515a0d6cc962a56d783ee4f5647a2ddc2d104491f0586a8ea0b8acebe0a203190f4f5567b349123dfd4c181bcc63361174a8ab63
f1c33ee4ac tests: implement BIP341 test vectors (Pieter Wuille)
ac3037df11 tests: BIP341 test vector generation (Pieter Wuille)
ca83ffc2ea tests: add deterministic signing mode to ECDSA (Pieter Wuille)
c98c53f20c tests: abstract out precomputed BIP341 signature hash elements (Pieter Wuille)
a5bde018b4 tests: give feature_taproot access to sighash preimages (Pieter Wuille)
5140825096 tests: add more fields to TaprootInfo (Pieter Wuille)
2478c6730a Make signing follow BIP340 exactly w.r.t. aux randomness (Pieter Wuille)
Pull request description:
This PR adds code to `test/functional/feature_taproot.py` which runs through a (deterministic) scenario covering several aspects of the wallet side of BIP341 (scriptPubKey computation from keys/scripts, control block computation, key path spending), with the ability to output test vectors in mediawiki format based on this scenario. The generated tests are then also included directly in `src/test/script_tests.cpp` and `src/test/script_standard_tests.cpp`.
I intend to add these test vectors to BIP341 itself: https://github.com/bitcoin/bips/pull/1225
ACKs for top commit:
laanwj:
Code review ACK f1c33ee4ac
Tree-SHA512: fcf7109539cb214d3190516b205cd32d2b1b452f14aa66f4107acfaa8bfc7d368f626857f1935665a4342eabc0b9ee8aba608a7c0a2494bec0b498e723439c9d
fadc4c7272 test: Add txindex migration test (MarcoFalke)
Pull request description:
Test for #22626
ACKs for top commit:
theStack:
Tested ACK fadc4c7272🌁
Tree-SHA512: fc7133ef52826bf0d4fa2ac72c3f1bed4a185ff7492396552ff2cbf6531b053238039211a710cbb949379c56875cd7715f1ed49a514dd3b3f1b46554e3d4bef5
a9872e1478 test: remove unnecessary block rehashing prior to solving (Sebastian Falbesoner)
Pull request description:
Solving a block involves continously rehashing it,
c9dd5c8d6e/test/functional/test_framework/messages.py (L759-L764)
i.e. any extra `rehash` calls before are not necessary and can be dropped.
The instances were identified by searching for all block solving calls via `git grep "solve("` in `./test/functional/`. From 95 instances of `CBlock.solve()` calls, 20 contained an unnecessary rehashing instruction before that are removed in this PR.
ACKs for top commit:
brunoerg:
tACK a9872e1478
rajarshimaitra:
tACK a9872e1478
Tree-SHA512: 160092be717d0d250778b8ab091ebd77cc6865d2754ef150cf3b4d4ac7304d4bf3d2ebb61ec6b04a55040c8895b9fb4d28653ea4b099d56d90776c9111cf173f
The `create_block` helper accepts a list of txs that it includes in the
created block, hence we don't have to do that manually. Also, rehashing
before solving the block is not needed and can be removed.
The function `trueDummy` in feature_nulldummy.py is currently more
complicated than it needs to be. Rather than converting the scriptSig to
a CScript and looping through it to build a new scriptSig with the
modified push, simply directly replace the push of null (OP_0) with a
push of one (OP_TRUE/OP_1).
Note that on master, actually an element with the value of 0x51 is
pushed (0x0151...) -- this was very likely not intended, as 0x51 is the
script op-code for OP_TRUE, and also the function's name suggests that
the "true" value shall be pushed.
This does the following:
* Adds a rfc6979 argument to test_framework/key.py's sign_ecdsa to
select (deterministic) RFC6979-based nonce generation.
* Add a flag in feature_taproot.py's framework called "deterministic".
* Make the Schnorr signing in feature_taproot.py randomized by default,
reverting to the old deterministic (aux_rnd=0x0000...00) behavior
if the deterministic context flag is set.
* Make the ECDSA signing in feature_taproot.py use RFC6979-based nonces
when the deterministic context flag is set (keeping the old randomized
behavior otherwise).
22b44fc696 p2p: improve checkaddrman logging with duration in milliseconds (Jon Atack)
ec65bed00e log, timer: add LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE macro (Jon Atack)
325da75a53 log, timer: allow not repeating log message on completion (Jon Atack)
Pull request description:
This patch:
- updates the `logging/timer.h::Timer` class to allow not repeating the log message on completion
- adds a `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro that prints the descriptive message when logging the start but not when logging the completion
- updates the checkaddrman logging to log the duration, and renames the function like the `-checkaddrman` configuration option in order to prefix every log message with `CheckAddrman` instead of the longer, less pleasant, and different-from-checkaddrman `ForceCheckAddrman` (the Doxygen documentation on the function already makes clear that it is unaffected by `m_consistency_check_ratio`).
before
```
2021-09-21T18:42:50Z [opencon] Addrman checks started: new 64864, tried 1690, total 66554
2021-09-21T18:42:50Z [opencon] Addrman checks completed successfully
```
after
```
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
```
To test, build and run bitcoind with `-debug=addrman -checkaddrman=<n>` for a value of `n` in the range of, say, 10 to 40.
ACKs for top commit:
laanwj:
Code review ACK 22b44fc696
Tree-SHA512: 658c0dfaaa9d07092e2418f2d05007c58cc35be6593f22b3c592ce793334a885dd92dacc46bdeddc9d37939cf11174660a094c07c0fa117fbb282953aa45a94d
aa1a4c9204 Add file validation to savemempool RPC test (lsilva01)
871e64d22f Add filename to savemempool RPC result (lsilva01)
Pull request description:
Currently, if the user calls the `savemempool` RPC method, there is no way to know
where the file was created (unless the user knows internal implementation details).
This PR adds a return message stating the file name and path where the mempool was saved and changes `mempool_persist.py` to validate this new return message.
ACKs for top commit:
laanwj:
Code review ACK aa1a4c9204
Tree-SHA512: e8b1dd0a8976e5eb15f7476c9651e492d2c621a67e0b726721fa7a2ae0ddd272ee28b87a2d0c650bd635e07fa96bdefe77bece4deb6486ef3ee9a4f83423a840
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
facc352648 test: Implicitly sync after generate*, unless opted out (MarcoFalke)
Pull request description:
The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves:
* Noticing the failure
* Fetching and reading the log to determine the test case that failed
* Adding a `self.sync_all()` where it was forgotten
* Spamming out a pr and waiting for review, which is already sparse
Also, writing a linter to catch those is not possible, nor is review effective in finding these bugs prior to merge.
Fix all future intermittent races caused by a missing sync_block call by calling `sync_all` implicitly after each `generate*`, unless opted out. This ensures that the code is race-free (with regards to blocks) when the tests pass once, instead of our current approach where the code can never be guaranteed to be race-free.
There are some scripted-diff cleanups (see https://github.com/bitcoin/bitcoin/pull/22567), but they will be submitted in a follow-up to reduce the conflicts in this pull.
ACKs for top commit:
lsilva01:
tACK facc352 on Ubuntu 20.04
brunoerg:
tACK facc352648 on MacOS 11.6
Tree-SHA512: 046a40a066b4a3bd28a3077bd654fa8887442dd1f0ec6fd11671865809ef02376f126eb667a1320ebd67b6e372c78c00dbf8bd25d86ed86f1d9a25363103ed97
420695c193 contrib: recognize CJDNS seeds as such (Vasil Dimov)
f9c28330a0 net: take the first 4 random bits from CJDNS addresses in GetGroup() (Vasil Dimov)
29ff79c0a2 net: relay CJDNS addresses even if we are not connected to CJDNS (Vasil Dimov)
d96f8d304c net: don't skip CJDNS from GetNetworkNames() (Vasil Dimov)
c2d751abba net: take CJDNS into account in CNetAddr::GetReachabilityFrom() (Vasil Dimov)
9b43b3b257 test: extend feature_proxy.py to test CJDNS (Vasil Dimov)
508eb258fd test: remove default argument of feature_proxy.py:node_test() (Vasil Dimov)
6387f397b3 net: recognize CJDNS addresses as such (Vasil Dimov)
e6890fcb44 net: don't skip CJDNS from GetNetworksInfo() (Vasil Dimov)
e9d90d3c11 net: introduce a new config option to enable CJDNS (Vasil Dimov)
78f456c576 net: recognize CJDNS from ParseNetwork() (Vasil Dimov)
de01e312b3 net: use -proxy for connecting to the CJDNS network (Vasil Dimov)
aedd02ef27 net: make it possible to connect to CJDNS addresses (Vasil Dimov)
Pull request description:
CJDNS overview
=====
CJDNS is like a distributed, shared VPN with multiple entry points where every participant can reach any other participant. All participants use addresses from the `fc00::/8` network (reserved IPv6 range). Installation and configuration is done outside of applications, similarly to VPN (either in the host/OS or on the network router).
Motivation
=====
Even without this PR it is possible to connect two Bitcoin Core nodes through CJDNS manually by using e.g. `-addnode` in environments where CJDNS is set up. However, this PR is necessary for address relay to work properly and automatic connections to be made to CJDNS peers. I.e. to make CJDNS a first class citizen network like IPv4, IPv6, Tor and I2P.
Considerations
=====
An address from the `fc00::/8` network, could mean two things:
1. Part of a local network, as defined in RFC 4193. Like `10.0.0.0/8`. Bitcoin Core could be running on a machine with such address and have peers with those (e.g. in a local network), but those addresses are not relayed to other peers because they are not globally routable on the internet.
2. Part of the CJDNS network. This is like Tor or I2P - if we have connectivity to that network then we could reach such peers and we do relay them to other peers.
So, Bitcoin Core needs to be able to tell which one is it when it encounters a bare `fc00::/8` address, e.g. from `-externalip=` or by looking up the machine's own addresses. Thus a new config option is introduced `-cjdnsreacable`:
* `-cjdnsreacable=0`: it is assumed a `fc00::/8` address is a private IPv6 (1.)
* `-cjdnsreacable=1`: it is assumed a `fc00::/8` address is a CJDNS one (2.)
After setting up CJDNS outside of Bitcoin Core, a node operator only needs to enable this option.
Addresses from P2P relay/gossip don't need that because they are properly tagged as IPv6 or as CJDNS.
For testing
=====
```
[fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa]:8333
[fc68:7026:cb27:b014:5910:e609:dcdb:22a2]:8333
[fcb3:dc50:e1ae:7998:7dc0:7fa6:4582:8e46]:8333
[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333
[fcf2:d9e:3a25:4eef:8f84:251b:1b4d:c596]:8333
```
ACKs for top commit:
dunxen:
ACK 420695c
jonatack:
re-ACK 420695c193 per `git range-diff 23ae793 4fbff39 420695c`
laanwj:
Code review ACK 420695c193
Tree-SHA512: 21559886271aa84671d52b120fa3fa5a50fdcf0fcb26e5b32049c56fab0d606438d19dd366a9c8ce612d3894237ae6d552ead3338b326487e3534399b88a317a
80dc829be7 tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676 fees: Always round up fee calculated from a feerate (Andrew Chow)
Pull request description:
When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.
A test is added for the assertion, along with a comment explaining what happens.
It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.
ACKs for top commit:
ryanofsky:
Code review ACK 80dc829be7
promag:
Tested ACK 80dc829be7.
lsilva01:
tACK 80dc829
meshcollider:
utACK 80dc829be7
Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
The default bool argument makes it harder to read because the last but
one argument is also bool. Pass all of them as named arguments to
increase readability.
Another bool argument will be added to indicate whether to test CJDNS.
Co-authored-by: Jon Atack <jon@atack.com>
Unless `-deprecatedrpc=fees` is passed, top level
fee fields are no longer returned for mempool entries.
Add instructions to field help on how to access
deprecated fields, update help text for readability,
and include units. This is important to help
avoid any confusion as users move from deprecated
fields to the fee fields object (credit: jonatack).
This affects `getmempoolentry`, `getrawmempool`,
`getmempoolancestors`, and `getmempooldescendants`
Modify `test/functional/mempool_packages.py` and
`test/functional/rpc_fundrawtransaction.py` tests
to no longer use deprecated fields.
Co-authored-by: jonatack <jon@atack.com>
61ec0539b2 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7b [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68 [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)
Pull request description:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
ACKs for top commit:
naumenkogs:
ACK 61ec0539b2
mzumsande:
ACK 61ec0539b2
shaavan:
ACK 61ec0539b2
Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
d891ae7681 Introduce new V4 format addrman (Pieter Wuille)
Pull request description:
#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.
Introduce a `V4_MULTIPORT` format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.
ACKs for top commit:
naumenkogs:
ACK d891ae7681
ajtowns:
utACK d891ae7681
vasild:
ACK d891ae7681
Tree-SHA512: de2153beb59152504ee0656dd0cc0b879b09136eb07e3ce0426d2fea778adfabacebbce5cf1a9a65dc99ad4e99cda42ab26743fe672fb82a9fbfec49c4cccb4d
d2c4904ef7 test: MiniWallet: more deterministic coin selection for coinbase UTXOs (oldest first) (Sebastian Falbesoner)
Pull request description:
The coin selection strategy for MiniWallet is quite straight-forward: simply pick a single UTXO with the largest value:
ab25ef8c7f/test/functional/test_framework/wallet.py (L173-L174)
If there are several candidates with the same value, however, it is not clear which one is taken. This can be particularly problematic for coinbase outputs with fixed block subsidy, since spending could lead to a `bad-txns-premature-spend-of-coinbase` reject if an UTXO from a too-recent block is picked. Introduce block height as second criteria (saved in `self._utxos` in the methods `generate(...)` and `rescan_utxos(...)`), in order to avoid potential issues with coinbases that are not matured yet. If there is a tie between coinbase UTXOs and non-coinbase UTXOs (the latter are added via `scan_tx(...)`), prefer the non-coinbase UTXOs, since those don't need to mature.
The issue came up while refactoring the test rpc_blockchain.py, see https://github.com/bitcoin/bitcoin/pull/23371#discussion_r737401936 (PR #23371).
ACKs for top commit:
MarcoFalke:
review ACK d2c4904ef7
shaavan:
ACK d2c4904ef7
Tree-SHA512: 15d67b42fb8b77fd53022ea2ab8a6ed2b615567f3ce73bab16c06bfcb687c1a04dcb0360d0c2287c526b604cd3ac5eef7b14ce46fc31e23047ce1a3290027306
2600db6c36 test: fix misleading fee unit in mempool_limit.py (Sebastian Falbesoner)
Pull request description:
The PR is a follow-up to #22543. The helper `send_large_txs` in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet's `create_self_transfer` method, which can't know that we append several tx outputs after, increasing the tx's vsize and decreasing it's fee rate accordingly.
In our case, the fee rate is off by several orders of magnitude, as the tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. It was suggested to simply in-line this helper as it's currently only used in this single test (https://github.com/bitcoin/bitcoin/pull/22543#discussion_r701685136, https://github.com/bitcoin/bitcoin/pull/22543#issuecomment-918986896), but I could imagine that this helper may also become useful for other tests and may be moved to a library (e.g. wallet.py) in the future.
Clarify the interface by passing an absolute fee that is deducted in the end (and also verified, via testmempoolaccept) and also describe how we come up with the value passed. On master, the comment says that the fee rate needs to increased "massively"; this word is also removed because the fee rate only needs to be higher for the test to succeed.
ACKs for top commit:
stratospher:
ACK 2600db6.
Tree-SHA512: 0bfacc3fa87603970d86c1d0186e51511f6c20c64b0559e19e7e12a68647f79dcb4f436000dee718fd832ce6a68e3bbacacb29145e0287811f1cb03d2f316843
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
The coin selection strategy for MiniWallet is quite straight-forward: simply
pick a single UTXO with the largest value:
self._utxos = sorted(self._utxos, key=lambda k: k['value'])
utxo_to_spend = utxo_to_spend or self._utxos.pop()
If there are several candidates with the same value, however, it is not clear
which one is taken. This can be particularly problematic for coinbase outputs
with fixed block subsidy, since spending could lead to a
'bad-txns-premature-spend-of-coinbase' reject if an UTXO from a too-recent
block is picked. Introduce block height as second criteria (saved in
self._utxos in the methods generate(...) and rescan_utxos(...)), in order to
avoid potential issues with coinbases that are not matured yet.
92617b7a75 effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.
Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
e9ade032f3 tests: Add feature_segwit.py --descriptors to test_runner.py (Andrew Chow)
ae6cbcc909 tests: restrict feature_segwit legacy wallet import tests (Andrew Chow)
1d13c44a4c tests: Use descriptors for feature_segwit multisig setup (Andrew Chow)
Pull request description:
`feature_segwit.py` has tests for some legacy wallet behavior, but otherwise does not really need the legacy wallet. Those parts now require `--legacy-wallet` to be provided (as with other legacy wallet tests). Other parts of the test are changed to work with descriptor wallets as well.
ACKs for top commit:
lsilva01:
tACK e9ade03
Tree-SHA512: 4ae76a4d2a8d318e7f8ad4c984748e3cdd67ed00359fcd798a08492ed9399b1d01be88c9ebea3d6c996fbe190cf41708a15c98f088f0cb5c47d6d00fff258944
45827fd718 test: check for block reject reasons in p2p_segwit.py [2/2] (Sebastian Falbesoner)
4eb532ff8b test: check for block reject reasons in p2p_segwit.py [1/2] (Sebastian Falbesoner)
b1488c4dce test: fix reference to block processing test in p2p_segwit.py (Sebastian Falbesoner)
Pull request description:
In the test `p2p_segwit.py`, there are many instances where we send a segwit block to a node with the expectation that it is rejected. For this purpose, the helper function `test_witness_block` exists which allows also to check for a specific reject `reason` that is asserted in the debug log:
502d22ceed/test/functional/p2p_segwit.py (L119-L120)
This PR aims to increase the precision of the tests by adding the expected reject reasons to all `test_witness_block` call instances (found via `grep`ing after `test_witness_block(.*accepted=False`). For some blocks that are rejected due to failed script verification, the exact reason is only shown in the debug log if parallel script verification is disabled. For this reason, the addition of the reasons is split up in two commits:
* first, all instances are tackled that are not subject to script verification, i.e. it doesn't matter whether parallel script verification is enabled or not (e.g. `bad-blk-weight`, `bad-witness-merkle-match`...)
* then, we explicitely set `-par=1` to only use one script thread, and add the remaining reasons (`non-mandatory-script-verify-flag` with the more specific reason in parantheses)
ACKs for top commit:
stratospher:
tested ACK 45827fd.
Tree-SHA512: 00f31867f11d48b38a42b1e79a1303bda1c797ccf3d8c73e6107d70b062604d51ee2a3f2251e7f068dfafdaf09469d27dfee438d9bc9ebaef7febc4b6ef90a95
66f6efc70a rpc: improve TransactionDescriptionString() "generated" help (Jon Atack)
296cfa312f test: add listtransactions/listsinceblock "trusted" coverage (Jon Atack)
d95913fc43 rpc: fix "trusted" description in TransactionDescriptionString (Jon Atack)
Pull request description:
The RPC gettransaction, listtransactions, and listsinceblock helps returned by `TransactionDescriptionString()` inform the user that the `trusted` boolean field is only present if the transaction is trusted and safe to spend from.
The field is in fact returned by `WalletTxToJSON()` when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false.
This patch fixes the help, adds test coverage, and touches up the help for the neighboring `generate` field.
ACKs for top commit:
rajarshimaitra:
tACK 66f6efc70a
theStack:
Tested ACK 66f6efc70a
Tree-SHA512: 4c2127765b82780e07bbdbf519d27163d414d9f15598e01e02210f210e6009be344c84951d7274e747b1386991d4c3b082cd25aebe885fb8cf0b92d57178f68e
9c1052a521 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad40463 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK 9c1052a521 on Ubuntu 20.04
prayank23:
tACK 9c1052a521
meshcollider:
Code review ACK 9c1052a521
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
fa6c62f34b test: Replace log with assert_equal in wallet_abandonconflict (MarcoFalke)
Pull request description:
This will make the test fail as soon as the bug is fixed, forcing it to
be updated. Otherwise, the bug might be fixed (or made worse)
accidentally, leaving the test in an inconsistent state.
ACKs for top commit:
theStack:
Concept and code-review ACK fa6c62f34b
brunoerg:
tACK fa6c62f34b
Tree-SHA512: 416f72380164bf3f93332a5cfa81a8e61d8ada3714ef6815889ed3cf2d16c09411760dee4acf19629227e565b765b3dea491a0b23efd38eb370254cfadf7c441
fadf1186c8 p2p: Use mocktime for ping timeout (MarcoFalke)
Pull request description:
It is slightly confusing to use mocktime for some times, but not others.
Start fixing that by making the ping timeout use mocktime.
The only downside would be that tests that use mocktime disconnect peers after this patch. However, I don't think this is an issue, as the inactivity check is already disabled for all functional tests after commit 6d76b57ca0. Only one unit test needed the inactivity check disabled as part of this patch.
A nice side effect of this patch is that the `p2p_ping` functional test now runs 4 seconds faster.
ACKs for top commit:
laanwj:
Code review ACK fadf1186c8
Tree-SHA512: e9e7b21040a89d9d574b3038f85a67e6336de6cd6e41aa286769cd03cada6e75a94ec01700e052e56d822ef85d7813cc06bf7e67b81543eff8917a16cdccf942
This will make the test fail as soon as the bug is fixed, forcing it to
be updated. Otherwise, the bug might be fixed (or made worse)
accidentally, leaving the test in an inconsistent state.
b7884dd1b6 test: bip125-replaceable in listsinceblock (brunoerg)
Pull request description:
This PR adds test coverage for bip125-replaceable in listsinceblock. I added this test into wallet_listtransactions.py instead of putting it into wallet_listsinceblock.py to utilize the scenario already created in wallet_listtransactions.py and avoid repetition.
ACKs for top commit:
theStack:
ACK b7884dd1b6
promag:
ACK b7884dd1b6.
stratospher:
tested ACK b7884dd. Verified the bip125-replaceable status of some transactions with listsinceblock.
lsilva01:
tACK b7884dd on Ubuntu 20.04
Tree-SHA512: 510dfe5a6f9d68e5a656514d356dc8fe99324296ed8caa78f0eb4b6c6906cf70b1fb50bde80aa6f61d726b2fa1d4ce1fe48c635ce24285588e56ceff92291617
4ac8c89ad9 test: check that bumpfee RPC fails for txs with descendants in mempool (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the bumpfee RPC error _"Transaction has descendants in the mempool"_,
6419bdfeb1/src/wallet/feebumper.cpp (L29-L32)
which is thrown if the bumped tx has descendants in the mempool and is _not_ connected to the bitcoin wallet (for those, the error "Transaction has descendants in the Wallet" is thrown a few lines above). To achieve that, the test framework's MiniWallet is used.
ACKs for top commit:
brunoerg:
tACK 4ac8c89ad9
promag:
Code review ACK 4ac8c89ad9. Nice stuff!
lsilva01:
Tested ACK 4ac8c89 cad756b10c9dee2d9e1405 on Ubuntu 20.04.
stratospher:
tested ACK 4ac8c89.
Tree-SHA512: 83e99f9dd2b140c0c0597c0c36c9c948fa334871be40e58a5e004440698d9685661c69bb83ab937d30f692545a3799705f991b31904f2ef31a2fbc3ae1179fa8
130ee48108 test: get and decode tx with a single `gettransaction` RPC call (Sebastian Falbesoner)
Pull request description:
Rather than subsequently calling `gettransaction` and `decoderawtransaction` to get the decoded information for a specific tx-id, we can simply use the verbose version of `gettransaction`, which returns this in a 'decoded' key. I.e.
```
node.decoderawtransaction(node.gettransaction(txid)['hex'])
```
can simply be replaced by:
```
node.gettransaction(txid=txid, verbose=True)['decoded']
```
Rationale: shorter code, shorter test logs, less RPC calls.
ACKs for top commit:
stratospher:
tested ACK 130ee48
amadeuszpawlik:
tACK 130ee48108
lsilva01:
Tested ACK 130ee48 on Ubuntu 20.04.
shaavan:
ACK 130ee48108
Tree-SHA512: cf0bd26e1e21b8022fb8062857906e0706f0ee32d3277f985c461e2519405afe445ab005f5f763fb268c7b4d6e48b2d47eda7af8621b3bce67cece8dfc9bc153
b65a25a846 log: improve addrman logging (Martin Zumsande)
Pull request description:
The addrman helper functions `GetNewBucket()` and `GetTriedBucket()`
1) log into the wrong category (`BCLog::NET` instead of `BCLog::ADDRMAN`)
2) log too unspecifically - especially `GetTriedBucket()` gets called from many different places (e.g. `Check_()`, `Serialize()`), it seems sufficient to me logging these when moving an address from new to tried. Running a node with `-checkaddrman=1`and net logging currently results in a lot of repetitive log entries.
This PR moves these log entries to `Add_()` and `Good_()` and also adds logging for `Select_()` (allowing statistics about New/Tried success probabilities), `GetAddr_()`, `ClearNew()` and `MakeTried()`.
ACKs for top commit:
jnewbery:
ACK b65a25a846
vasild:
ACK b65a25a846
Tree-SHA512: 90ab0f64eb44b7388a198efccb613577b74989fea73194bda7de8bfbd50bdb19127cb12f5ec645c7859afdb89290614a79e255f3af0a63a58d4f21aa8fe7b696
5c34507ecb core_write: Rename calculate_fee to have_undo for clarity (fyquah)
8edf6204a8 release-notes: Add release note about getblock verbosity level 3. (fyquah)
459104b2aa rest: Add test for prevout fields in getblock (fyquah)
4330af6f72 rpc: Add test for level 3 verbosity getblock rpc call. (fyquah)
51dbc167e9 rpc: Add level 3 verbosity to getblock RPC call. (fyquah)
3cc95345ca rpc: Replace boolean argument for tx details with enum class. (fyquah)
Pull request description:
Author of #21245 expressed [time issues](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-902332088) in the original PR. Given that #21245 has received a lot of review*, I have decided to open this new pull request with [modifications required to get ACK from luke-jr ](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-905150806) and a few nits of mine.
### Original PR description
> Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing `/rest/block` API by adding a `prevout` fields to tx inputs. This is mentioned in the change to the release notes.
>
> I added some functional tests that
>
> * checks that the RPC call still works when TxUndo can't be found
>
> * Doesn't display the "value" or "scriptPubKey" of the previous output when at a lower verbosity level
>
>
> This "completes" the issue #18771
### Possible improvements
* b0bf4f255f - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-894853784 for more context.
### Examples
Examples of the `getblock` output with various verbose levels. Note that `000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5` contains only 2 transactions.
#### Verbose level 0
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0
```
##### Verbose level 1
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1
```
##### Verbose level 2
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2
```
##### Verbose level 3
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3
```
#### REST
```bash
curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json
```
<sub>* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.</sub>
Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message.
ACKs for top commit:
0xB10C:
ACK 5c34507ecb
meshcollider:
utACK 5c34507ecb
theStack:
ACK 5c34507ecb👘
promag:
Concept ACK 5c34507ecb
Tree-SHA512: bbff120d8fd76e617b723b102b0c606e0d8eb27f21c631d5f4cdab0892137c4bc7c65b1df144993405f942c91be47a26e80480102af55bff22621c19f518aea3
9de0d94508 doc: add disclaimer highlighting shortcomings of the basic multisig example (Michael Dietz)
f9479e4626 test, doc: basic M-of-N multisig minor cleanup and clarifications (Michael Dietz)
e05cd0546a doc: add another signing flow for multisig with descriptor wallets and PSBTs (Michael Dietz)
17dd657300 doc: M-of-N multisig using descriptor wallets and PSBTs, as well as a signing flow (Michael Dietz)
1f20501efc test: add functional test for multisig flow with descriptor wallets and PSBTs (Michael Dietz)
Pull request description:
Aims to resolve issue https://github.com/bitcoin/bitcoin/issues/21278. I try to follow the steps laanwj outlined there exactly, with the exception of using `combinepsbt` instead of `joinpsbts`. I wrote a functional test to make sure it works as expected before doing the docs, and figured it would also be a good source of documentation. So I kept the test as simple as possible and didn't go crazy with edge-cases and various checks. I do have a lot more test-cases I've written that I will follow up with (either in a separate PR or another commit - lmk if you have a preference), but I want to do it in a way that doesn't bloat this test so it remains useful as a quickstart (unless that's a bad idea)?
ACKs for top commit:
S3RK:
Code review ACK 9de0d94. Rspigler's argument convinced me that we should leave the workflow with two wallets. I assume using multisig with external signers is a popular use-case and it's important to keep compatibility.
laanwj:
Code and documentation review ACK 9de0d94508
Tree-SHA512: 6c76e787c21f09d8be5eaa11f3ca3eaa4868497824050562bdfb2095c73b90f5e8987a8775119891d6bfde586e3f31ad1b13e4b67b0802e1d23ef050227a1211
fac62e6ff5 test: Delete generate* calls from TestNode (MarcoFalke)
fac7f6102f test: Use generate* node RPC, not wallet RPC (MarcoFalke)
faac1cda6e test: Use generate* from TestFramework, not TestNode (MarcoFalke)
Pull request description:
Deleting the methods is needed for #22567 to pave the way to make it easier to implicitly call the `sync_all` member function.
Without the methods being deleted, nothing prevents developers from adding calls to it. As history showed, developers *will* add calls to it. For example, see commit eb02dbba3c from today or the first commit in this pull request.
ACKs for top commit:
stratospher:
Tested ACK fac62e6.
brunoerg:
tACK fac62e6ff5
promag:
Code review ACK fac62e6ff5.
Tree-SHA512: 6d4dea8f95ead954acfef2e6a5d98897ce0c2d02265c5b137bb149d0265543bd51d7e8403e1945b9af75df5524ca50064fe1d2a432b25c8abc71bbb28ed6ed53
bda620aecd test: check abandoned tx in listsinceblock (brunoerg)
Pull request description:
This PR tests if the abandoned transaction is correct in listsinceblock return (wallet_abandonconflict.py).
ACKs for top commit:
jonatack:
ACK bda620aecd
theStack:
ACK bda620aecd
stratospher:
Tested ACK bda620a. This PR verifies whether the transaction txAB1 has been abandoned in listsinceblock and is a nice addition to the test!
Tree-SHA512: e4dce344cf621de7a8b5bd8660d252419772a293080fc881f6f448b6df85c6b1c8f0df619e855a40b6393f53c836f0d7fadbd3916c20cccd3a95830b8b502991
Rather than subsequently calling `gettransaction` and
`decoderawtransaction` to get the decoded information for a specific
tx-id, we can simply use the verbose version of `gettransaction`, which
returns this in a 'decoded' key. I.e.
node.decoderawtransaction(node.gettransaction(txid)['hex'])
can be replaced by:
node.gettransaction(txid=txid, verbose=True)['decoded']
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
This commit adds missing test coverage for the bumpfee RPC
error "Transaction has descendants in the mempool", which is
thrown if the bumped tx has descendants in the mempool and is
_not_ connected to the bitcoin wallet. To achieve that, the
test framework's MiniWallet is used.
6531599f42 test: Add check that newkeypool flushes change addresses too (Samuel Dobson)
84fa19c77a Add release notes for keypool flush changes (Samuel Dobson)
f9603ee4e0 Add test for flushing keypool with newkeypool (Samuel Dobson)
6f6f7bb36c Make legacy wallet upgrades from non-HD to HD always flush the keypool (Samuel Dobson)
2434b10781 Fix outdated keypool size default (Samuel Dobson)
22cc797ca5 Add newkeypool RPC to flush the keypool (Samuel Dobson)
Pull request description:
This PR makes two main changes:
1) Adds a new RPC `newkeypool` which will entirely flush and refill the keypool.
2) When upgradewallet is called on old, non-HD wallets upgrading them to HD, we now always flush the keypool and generate a new one, to immediately start using the HD generated keys.
This PR is motivated by a number of users with old, pre-compressed-key wallets upgrading them and being confused about why they still can't generate p2sh-segwit or bech32 addresses -- this is due to uncompressed keys remaining in the keypool post-upgrade and being illegal in these newer address formats. There is currently no easy way to flush the keypool other than to call `getnewaddress` a hundred/thousand times or an ugly hack of using a `sethdseed` call.
ACKs for top commit:
laanwj:
re-ACK 6531599f42
meshcollider:
Added new commit 6531599f42 to avoid invalidating previous ACKs.
instagibbs:
ACK 6531599f42
Tree-SHA512: 50c79c5d42dd27ab0ecdbfdc4071fdaa1b2dbb2f9195ed325b007106ff19226419ce57fe5b1539c0c24101b12f5e034bbcfb7bbb0451b766cb1071295383d774
b5950dd59c validation: put coins cache write log into bench debug log (Anthony Towns)
31b2b802b5 blockstorage: use debug log category (Anthony Towns)
da94ebc2fa validation: move header validation error logging to VALIDATION debug category (Anthony Towns)
1d7d835ec3 validation: include block hash when reporting prev block not found errors (Anthony Towns)
Pull request description:
Moves the following log messages into debug log categories:
* "AcceptBlockHeader: ..." to validation
* "Prune: deleted blk/rev" to new blockstorage log category
* "Leaving block file" moves from validation to blockstorage
* "write coins cache to disk" to bench
Also adds the hash of the block to the log message when AcceptBlockHeader is rejecting because of problems with the prev block.
ACKs for top commit:
practicalswift:
cr ACK b5950dd59c
Empact:
Code review ACK b5950dd59c
laanwj:
Code review ACK b5950dd59c
promag:
Code review ACK b5950dd59c.
meshcollider:
Code review ACK b5950dd59c
Tree-SHA512: a73fdbfe8d36da48a3e89c2d5e0b6a3c5045d280c1a57f61c38d0d21f4f198aece4bd85155be3439e179d5dabdb523bf15fa0395e0e3ceff19c878ba3112c840
Because of floating point precision issues, not all of the rounding done
is always correct. To fix this, the fee calculation for
assert_fee_amount is changed to better reflect how CFeeRate::GetFee does
it.
First the feerate is converted to an int representing sat/kvb. Then this
is multiplied by the transaction size, divivided by 1000, and rounded up
to the nearest sat. The result is then converted back to BTC (divided by
1e8) and then rounded down to the nearest sat to avoid precision errors.
c2fbdca549 Add BECH32_INVALID_VERSION test (lsilva01)
b142f79ddb skip test_getaddressinfo() if wallet is disabled (lsilva01)
Pull request description:
Most of `test/functional/rpc_invalid_address_message.py` does not requires wallet.
But if the project is compiled in disable-wallet mode, the entire test will be skipped.
This PR changes the test to run the RPC tests first and then checks if the wallet is compiled.
ACKs for top commit:
stratospher:
tested ACK c2fbdca
Tree-SHA512: 11fa2fedf4a15aa45e3f12490df8e22290a867d5de594247211499533c32289c68c0b60bd42dbf8305e43dbcc042789e7139317ef5c9f8cf386f2d84c91b9ac2
When calculating a txs absolute fee, if the fee is rounded down to the
nearest satoshi, it is possible for the coin selection algorithms to
undercalculate the fee needed. This can lead to an assertion error in
some situations. One such scenario is added to
rpc_fundrawtransaction.py.
When calculating the fee for a given tx size from a fee rate, we should
always round up to the next satoshi. Otherwise, if we round down (via
truncation), the calculated fee may result in a fee with a feerate
slightly less than targeted.
This is particularly important for coin selection as a slightly lower
feerate than expected can result in a variety of issues.
429b49378e test: introduce script_util helper for creating P2PK scripts (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #22363, which took use of already existing `script_util` helpers to get rid of manual CScript for the P2{PKH,SH,WPKH,WSH} output types, in order to increase readability and maintainability of the test code. Here the same is done for P2PK scripts by introducing a helper `key_to_p2pk_script` and using it. Note that the helper only accepts ECDSA pubkeys (i.e. ones with a size of 33 or 65 bytes), hence it can't be used for scripts in the form of [x-only-pubkey, OP_CHECKSIG].
ACKs for top commit:
brunoerg:
Code review ACK 429b49378e
laanwj:
Code review ACK 429b49378e
rajarshimaitra:
Concept + tACK 429b49378e
Tree-SHA512: 984aea01eba5f38a328d69905d90a3a36f0a02419ca3e5baf3c8095895fb094e3780c7da16fad5851db3847bdb05ce8cda244ab09b79b8aa9602dfb3c5e0414c
3b613722f6 Add release notes for fee est with replacement txs (Antoine Poinsot)
4556406562 qa: test fee estimation with replacement transactions (Antoine Poinsot)
053415b297 qa: split run_test into smaller parts (Antoine Poinsot)
06c5ce9714 Re-include RBF replacement txs in fee estimation (Antoine Poinsot)
Pull request description:
This effectively reverts #9519.
RBF is now largely in use on the network (signaled for by around 20% of
all transactions on average) and replacement logic is implemented in
most end-user wallets. The rate of replaced transactions is also
expected to rise as fee-bumping techniques are being developed for
pre-signed transaction ("L2") protocols.
ACKs for top commit:
prayank23:
reACK 3b613722f6
Zero-1729:
re-ACK 3b613722f6
benthecarman:
reACK 3b613722f6
glozow:
ACK 3b613722f6
theStack:
re-ACK 3b613722f6🍪
Tree-SHA512: a6146d15c80ff4ba9249314b0ef953a66a15673e61b8f98979642814f1b169b5695e330e3ee069fa9a7e4d1f8aa10e1dcb7f9aa79181cea5a4c4dbcaf5483023
502f50da12 Test transactions conflicted by double spend in listtransactions (Jon Atack)
Pull request description:
Test the properties of transactions conflicted by a double spend as returned by RPC listtransactions in the abandoned, confirmations, trusted and walletconflicts fields. These fields are also returned by RPCs listsinceblock and gettransactions.
ACKs for top commit:
brunoerg:
tACK 502f50da12
rajarshimaitra:
Concept + tACK 502f50da12
Tree-SHA512: 28968f4a5f1960ea45b2e6f5b20fe25c1b51f66944062dcddea52ea970ad21c74d583793d091b84e8a5e506d6aecc1f0435c5b918213975b22c38e02bba19aa1
fa2ac5881e test: Replace satoshi_round with int() or Decimal() (MarcoFalke)
Pull request description:
satoshi_round will round down. To make the code easier to parse use
Decimal() where possible, which does not round. Or use int(), which
explicitly rounds down.
ACKs for top commit:
lsilva01:
Tested ACK fa2ac5881e on Ubuntu 20.04.
Tree-SHA512: 17795d906aa7652933d43e510e993cdd9cf8926da1febf1c42d463048cb38c92dc518ec08736efe29c0189ffd532b108bc7a715f32b4c2ee58b215df65352eb9
satoshi_round will round down. To make the code easier to parse use
Decimal() where possible, which does not round. Or use int(), which
explicitly rounds down.
and update the function name to CheckAddrman (drop "Force") for
nicer log output as it is prefixed to each of these log messages:
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
The existing Doxygen documentation on the function already makes
clear that it is unaffected by m_consistency_check_ratio.
021f86953e [style] Run changed files through clang formatter. (Amiti Uttarwar)
375750387e scripted-diff: Rename CAddrInfo to AddrInfo (Amiti Uttarwar)
dd8f7f2500 scripted-diff: Rename CAddrMan to AddrMan (Amiti Uttarwar)
3c263d3f63 [includes] Fix up included files (Amiti Uttarwar)
29727c2aa1 [doc] Update comments (Amiti Uttarwar)
14f9e000d0 [refactor] Update GetAddr_() function signature (Amiti Uttarwar)
40acd6fc9a [move-only] Move constants to test-only header (Amiti Uttarwar)
7cf41bbb38 [addrman] Change CAddrInfo access (Amiti Uttarwar)
e3f1ea659c [move-only] Move CAddrInfo to test-only header file (Amiti Uttarwar)
7cba9d5618 [net, addrman] Remove external dependencies on CAddrInfo objects (Amiti Uttarwar)
8af5b54f97 [addrman] Introduce CAddrMan::Impl to encapsulate addrman implementation. (Amiti Uttarwar)
f2e5f38f09 [move-only] Match ordering of CAddrMan declarations and definitions (Amiti Uttarwar)
5faa7dd6d8 [move-only] Move CAddrMan function definitions to cpp (Amiti Uttarwar)
Pull request description:
Introduce the pimpl pattern for AddrMan to separate the implementation details from the externally used object representation. This reduces compile-time dependencies and conceptually clarifies AddrMan's interface from the implementation specifics.
Since the unit & fuzz tests currently rely on accessing AddrMan internals, this PR introduces addrman_impl.h, which is exclusively imported by addrman.cpp and test files.
ACKs for top commit:
jnewbery:
ACK 021f86953e
GeneFerneau:
utACK [021f869](021f86953e)
mzumsande:
ACK 021f86953e
rajarshimaitra:
Concept + Code Review ACK 021f86953e
theuni:
ACK 021f86953e
Tree-SHA512: aa70cb77927a35c85230163c0cf6d3872382d79048b0fb79341493caa46f8e91498cb787d8b06aba4da17b2f921f2230e73f3d66385519794fff86a831b3a71d
4747da3a5b Add syscall sandboxing (seccomp-bpf) (practicalswift)
Pull request description:
Add experimental syscall sandboxing using seccomp-bpf (Linux secure computing mode).
Enable filtering of system calls using seccomp-bpf: allow only explicitly allowlisted (expected) syscalls to be called.
The syscall sandboxing implemented in this PR is an experimental feature currently available only under Linux x86-64.
To enable the experimental syscall sandbox the `-sandbox=<mode>` option must be passed to `bitcoind`:
```
-sandbox=<mode>
Use the experimental syscall sandbox in the specified mode
(-sandbox=log-and-abort or -sandbox=abort). Allow only expected
syscalls to be used by bitcoind. Note that this is an
experimental new feature that may cause bitcoind to exit or crash
unexpectedly: use with caution. In the "log-and-abort" mode the
invocation of an unexpected syscall results in a debug handler
being invoked which will log the incident and terminate the
program (without executing the unexpected syscall). In the
"abort" mode the invocation of an unexpected syscall results in
the entire process being killed immediately by the kernel without
executing the unexpected syscall.
```
The allowed syscalls are defined on a per thread basis.
I've used this feature since summer 2020 and I find it to be a helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.
---
Quick start guide:
```
$ ./configure
$ src/bitcoind -regtest -debug=util -sandbox=log-and-abort
…
2021-06-09T12:34:56Z Experimental syscall sandbox enabled (-sandbox=log-and-abort): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.
…
2021-06-09T12:34:56Z Syscall filter installed for thread "addcon"
2021-06-09T12:34:56Z Syscall filter installed for thread "dnsseed"
2021-06-09T12:34:56Z Syscall filter installed for thread "net"
2021-06-09T12:34:56Z Syscall filter installed for thread "msghand"
2021-06-09T12:34:56Z Syscall filter installed for thread "opencon"
2021-06-09T12:34:56Z Syscall filter installed for thread "init"
…
# A simulated execve call to show the sandbox in action:
2021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
…
Aborted (core dumped)
$
```
---
[About seccomp and seccomp-bpf](https://en.wikipedia.org/wiki/Seccomp):
> In computer security, seccomp (short for secure computing mode) is a facility in the Linux kernel. seccomp allows a process to make a one-way transition into a "secure" state where it cannot make any system calls except exit(), sigreturn(), and read() and write() to already-open file descriptors. Should it attempt any other system calls, the kernel will terminate the process with SIGKILL or SIGSYS. In this sense, it does not virtualize the system's resources but isolates the process from them entirely.
>
> […]
>
> seccomp-bpf is an extension to seccomp that allows filtering of system calls using a configurable policy implemented using Berkeley Packet Filter rules. It is used by OpenSSH and vsftpd as well as the Google Chrome/Chromium web browsers on Chrome OS and Linux. (In this regard seccomp-bpf achieves similar functionality, but with more flexibility and higher performance, to the older systrace—which seems to be no longer supported for Linux.)
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 4747da3a5b
Tree-SHA512: e1c28e323eb4409a46157b7cc0fc29a057ba58d1ee2de268962e2ade28ebd4421b5c2536c64a3af6e9bd3f54016600fec88d016adb49864b63edea51ad838e17
74c0d81b46 test: use miniwallet in test_rpc() function in feature_rbf.py (Shubhankar)
Pull request description:
This PR aims to further increase MiniWallet usage in the functional test feature_rbf.py by using it in the test_rpc(...) . Sub test needs wallet to get a utxo for creating raw txn, and to get address for output, address can be hardcoded and mini wallet can be used for utxo. fund raw transaction is a wallet rpc and should be tested only when bitcoin core is compiled with wallet
ACKs for top commit:
laanwj:
Code review ACK 74c0d81b46
theStack:
tACK 74c0d81b46
Tree-SHA512: a98cc0df0fe70ee113a293cd4fd659c5bece47c17937d368193b65b150b331a2c694cdeb2792386f1a6deefb8d6750ed83ffa0ae6f9d13fa2b7625cd80bc692a
928af61cdb allow send rpc take external inputs and solving data (Andrew Chow)
e39b5a5e7a Tests for funding with external inputs (Andrew Chow)
38f5642ccc allow fundtx rpcs to work with external inputs (Andrew Chow)
d5cfb864ae Allow Coin Selection be able to take external inputs (Andrew Chow)
a00eb388e8 Allow CInputCoin to also be constructed with COutPoint and CTxOut (Andrew Chow)
Pull request description:
Currently `fundrawtransaction` and `walletcreatefundedpsbt` both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
ACKs for top commit:
prayank23:
reACK 928af61cdb
meshcollider:
Re-utACK 928af61cdb
instagibbs:
crACK 928af61cdb
yanmaani:
utACK 928af61.
Tree-SHA512: bc7a6ef8961a7f4971ea5985d75e2d6dc50c2a90b44c664a1c4b0f1be5c1c97823516358fdaab35771a4701dbefc0862127b1d0d4bfd02b4f20d2befa4434700
Short options should only be a single character. If not, they can't be
concatenated in a single "-word" (from review by luke-jr).
F is chosen instead of f, because f could be reserved to the nested
wallet_hd.py (test_framework/test_framework.py) arguments parser.
Test the properties of transactions conflicted by a double
spend as returned by RPC listtransactions in the "abandoned",
"confirmations", "trusted" and "walletconflicts" fields.
These fields are also returned by RPCs listsinceblock and
gettransactions.
b658d7d5c5 test: update assert_fee_amount() in test_framework/util.py (Jon Atack)
Pull request description:
Follow-up to 42e1b5d979 (#12486).
- update call to `round()` with our utility function `satoshi_round()` to avoid intermittent test failures
- rename `fee_per_kB` to `feerate_BTC_kvB` for precision
- store division result in `feerate_BTC_vB`
Possibly resolves#19418.
ACKs for top commit:
meshcollider:
utACK b658d7d5c5
Tree-SHA512: f124ded98c913f98782dc047a85a05d3fdf5f0585041fa81129be562138f6261ec1bd9ee2af89729028277e75b591b0a7ad50244016c2b2fa935c6e400523183
18c5b23a0f [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge)
a79ad65fc2 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge)
5e231c116b [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge)
5bf6587457 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge)
0dc8bf5b92 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge)
Pull request description:
A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.
In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.
>**Example:**
>A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`.
## Approach
This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`.
A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.
ACKs for top commit:
naumenkogs:
ACK 18c5b23a0f
rajarshimaitra:
tACK 18c5b23a0f
jnewbery:
reACK 18c5b23a0f
theStack:
re-ACK 18c5b23a0f🥛
Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
dc3ec74d67 Add rescan removal release note (Samuel Dobson)
bccd1d942d Remove -rescan startup parameter (Samuel Dobson)
f963b0fa8c Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson)
6c006495ef Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson)
Pull request description:
Remove the `-rescan` startup parameter.
Rescans can be run with the `rescanblockchain` RPC.
Rescans are still done on wallet-load if needed due to corruption, for example.
ACKs for top commit:
achow101:
ACK dc3ec74d67
laanwj:
re-ACK dc3ec74d67
Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c
- update call to round() with satoshi_round() to avoid intermittent test failures
- rename fee_per_kB to feerate_BTC_kvB for precision
- store division result in feerate_BTC_vB
The helps for RPCs gettransaction, listtransactions, and
listsinceblock returned by TransactionDescriptionString()
state that the "trusted" boolean field is only present if the
transaction is trusted and safe to spend from.
The "trusted" boolean field is in fact returned by
WalletTxToJSON() when the transaction has 0 confirmations,
or negative confirmations, if conflicted, and it can be
true or false.
This commit updates TransactionDescriptionString() to a
more accurate description for "trusted" and updates the
existing line of test coverage to fail more helpfully.
b207971465 Fix feature_segwit failure due to witness (Samuel Dobson)
Pull request description:
Fixes#23116
The failure is due to sometimes spending segwit outputs, which add an additional 1 sigop in the witness, added to the 2 (*4) in the outputs.
ACKs for top commit:
brunoerg:
tACK b207971465
Tree-SHA512: 7e3c09d162e6941a3028514e95ae3aab2a3f858e2f056229c5faff5e37ece897dc63e3f8c0e07fad9fc4621b74a6dee5ff777e910b2b009a41bb6e72a09217bd
cfdb6baa22 test: use MiniWallet for p2p_filter.py (Sebastian Falbesoner)
6fc2cd3f09 test: introduce helper to create random P2WPKH scriptPubKeys (Sebastian Falbesoner)
aa26797f69 test: MiniWallet: add `send_to` method to create arbitrary txouts (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_filter.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
For this purpose, a MiniWallet method `send_to` is introduced first, which allows to create arbitrary outputs (scriptPubKey/amount). Note that the implementation for this is already present in feature_rbf.py (recently added in PR #22998), i.e. it is simply moved to the MiniWallet interface.
ACKs for top commit:
laanwj:
Code review ACK cfdb6baa22
Tree-SHA512: 13b063631f0d7af065b7757cfe8b47c9be6cb9850ac5db2968a2bba4f5a18cdc9f89173a9b03971545356225082042f5fdbe49d3036027d18e8b7eb042d04f5e
fa54efda9b test: pep-8 touched test (MarcoFalke)
fa46768059 test: Remove unused and confusing main parameter from script_util (MarcoFalke)
Pull request description:
ACKs for top commit:
fanquake:
ACK fa54efda9b
Tree-SHA512: 124e888e16c92bb24ab4d6f68a768d295500a48f8c6c0b4f069493effcd761f80be78dd93b31edbb529ebe4c8aaca764aaff48d1e3b23659dde722981dc787a5
240ea294d5 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21 test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e7 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
ea31caf6b4 update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (pranabp-bit)
Pull request description:
This PR is in response to the issue [#19699](https://github.com/bitcoin/bitcoin/issues/19699).
Based on the discussion in the comments of PR [#22673](https://github.com/bitcoin/bitcoin/pull/22673) changes have been made in the `estimatesmartfee` itself such that it takes into account `mempoolMinFee` and `relayMinFee` . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as [#16072](https://github.com/bitcoin/bitcoin/issues/16072).
The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality.
ACKs for top commit:
meshcollider:
re-utACK ea31caf6b4
Tree-SHA512: 8f36153a07cbd552c5c13d11d9c6e987a7a555ea4cc83f2573c0c92dd97c706d90c30a7248671437c2f3a836d3272f8fad53d15a5fa6efaa0409ae8009b0a18d
43cd6b8af9 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2c6c refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac7b8 refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638daa rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)
Pull request description:
Resolves#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.
`-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).
Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)
ACKs for top commit:
MarcoFalke:
re-ACK 43cd6b8af9🐉
meshcollider:
Code review ACK 43cd6b8af9
jonatack:
ACK 43cd6b8af9 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit
Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
b8909b0746 Run functional tests with all possible flags (Samuel Dobson)
Pull request description:
Functional tests which use flags like `--descriptors` or `--legacy-wallet` won't run if only the base script is given to `test_runner.py` because it doesn't match any script in the list exactly. It would be easier if it would just run both options.
For example, instead of:
```
test_runner.py 'wallet_basic.py --legacy-wallet' 'wallet_basic.py --descriptors'
```
We can now just run:
```
test_runner.py wallet_basic
```
Also useful for `--usecli`, the IPv4/IPv6/nonloopback `rpc_bind.py` variations, etc.
ACKs for top commit:
laanwj:
Code review ACK b8909b0746
MarcoFalke:
review ACK b8909b0746
Tree-SHA512: d367037cb170e705551726d47fe4569ebc3ceadece280dd3edbb3ecb41e19f3263d6d272b407316ed6011164e850df4321fb340b1b183b34497c9f7cc439f4d8
This will provide better estimates which would be closer to fee paid in actual
transactions.
The test has also been changed such that when the node is restarted with a
high mempoolMinFee, the estimatesmartfee still returns a feeRate greater
than or equal to the mempoolMinFee, minRelayTxFee.(just like the feeRate of actual transactions)
7e3ee4cdd0 GUI: Ask user to unlock wallet before signing psbt (Samuel Dobson)
0f3acecf33 Add test that walletprocesspsbt requires unlocked wallet when signing (Samuel Dobson)
0e895212bb Ensure wallet is unlocked before signing in walletprocesspsbt (Samuel Dobson)
Pull request description:
If signing a PSBT, we need to ensure the wallet is unlocked.
Fixes#22874, fixesbitcoin-core/gui#312
ACKs for top commit:
achow101:
ACK 7e3ee4cdd0
lsilva01:
Code Review ACK 7e3ee4cdd0
benthecarman:
ACK 7e3ee4cdd0
Tree-SHA512: 6726a873582747900ab454ea21153a92be86808a4c1517dc2856b389876a2da9e8df1ffa9b567b6bd017038342c3544ecf5ca3c97744e7debe0a5ee65563687d
With this new method, outputs to an arbitrary scriptPubKey/amount can
be created. Note that the implementation was already present in the
test feature_rbf.py and is just moved to the MiniWallet interface, in
order to enable other tests to also use it.
faa9c19a4b doc: Add 23061 release notes (MarcoFalke)
faff17bbde Fix (inverse) meaning of -persistmempool (MarcoFalke)
Pull request description:
Passing `-persistmempool` is currently treated as `-nopersistmempool`
ACKs for top commit:
jnewbery:
reACK faa9c19a4b
hebasto:
ACK faa9c19a4b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee
fad02274ba test: Remove Windows workaround in authproxy (MarcoFalke)
Pull request description:
Might no longer be needed after https://github.com/bitcoin/bitcoin/pull/23089
ACKs for top commit:
hebasto:
ACK fad02274ba, passed 3 consequential runs on my [personal CI](https://cirrus-ci.com/task/4897456077930496):
Tree-SHA512: 3c408e068ad7590dc0e5922c0b4cd3bf927e22fe624b13b8ab38db848342d310c9c934f358638fd5234c7b5f10f95d3bddc676deb925dcbba54945e2e8229275
5825b34783 test: avoid non-determinism in asmap-addrman test (Jon Atack)
Pull request description:
This is the same approach as for the addpeeraddress test in `test/functional/rpc_net.py` in commit 869f1368.
The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test still fails when expected:
- `git checkout 181a120 && git cherry-pick ef242f5`
- recompile bitcoind
- git checkout this branch and run `test/functional/feature_asmap.py`. Expected output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=
```
Closes#23078.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
ACKs for top commit:
mzumsande:
Re-ACK 5825b34783
Tree-SHA512: eb6624a196fa5617c84aad860c8f91e8a8f60fc9fe2d7168facc298ee38db5e93b7e988e11c2ac1b399dc2c6b8fad360fb10bdbf10e598a1878300388475a200
d96b000e94 Make GUI UTXO lock/unlock persistent (Samuel Dobson)
077154fe69 Add release note for lockunspent change (Samuel Dobson)
719ae927dc Update lockunspent tests for lock persistence (Samuel Dobson)
f13fc16295 Allow lockunspent to store the lock in the wallet DB (Samuel Dobson)
c52789365e Allow locked UTXOs to be store in the wallet database (Samuel Dobson)
Pull request description:
Addresses and closes#22368
As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.
Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.
ACKs for top commit:
achow101:
ACK d96b000e94
kristapsk:
ACK d96b000e94
lsilva01:
Tested ACK d96b000e94 on Ubuntu 20.04
prayank23:
ACK d96b000e94
Tree-SHA512: 957a5bbfe7f763036796906ccb1598feb6c14c5975838be1ba24a198840bf59e83233165cb112cebae909b6b25bf27275a4d7fa425923ef6c788ff671d7a89a8
fa04f26aa7 test: Avoid race after connect_nodes (MarcoFalke)
Pull request description:
Wait until the connection is fully established on both sides (verack). Fixes#22714
ACKs for top commit:
kiminuo:
utACK fa04f26aa7
Tree-SHA512: bc2c44b44b688086ff84046924cf5251dd625584e93ce8fa17de27023855b32f3bb55109b846abbcec775e2836c7f3c5a81d6b4aff7c4ac065b9aefa044c1883
This is the same approach as for the addpeeraddress test in
`test/functional/rpc_net.py` in commit 869f1368.
The probability of collision when adding an addrman entry is
expected to be 1/2^16 = 1/65536 for an address from a different /16.
This change hopes to avoid these collisions by adding 1 tried entry
before adding 1 new table one, instead of 2 tried entries followed
by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test stills fails when expected:
- git checkout 181a120 && git cherry-pick ef242f5
- recompile bitcoind
- git checkout this branch and run test/functional/feature_asmap.py. Expected output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=
```
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This commit adds specific expected reject reasons for segwit blocks
sent to the node, that are only showing up if one script threads
is used. For this reason, the node is started with the parameter
`-par=1`.
This commit adds specific expected reject reasons for segwit blocks
sent to the node, that are independent of whether multiple script threads
are activated or not.
fa4db8671b test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffd Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aa test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef539 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671b
theStack:
re-ACK fa4db8671b
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
b69a106bcd test: use test_framework.p2p P2P_SERVICES in functional tests (Jon Atack)
Pull request description:
`P2P_SERVICES` is defined in `test/functional/test_framework/p2p.py`, so we can use it as a single definition for our functional tests. It may also be a tiny bit more efficient to use the constant rather than calculating `NODE_NETWORK | NODE_WITNESS` every time we need it in the tests.
ACKs for top commit:
laanwj:
Code review ACK b69a106bcd
klementtan:
crACK b69a106bcd
fanquake:
ACK b69a106bcd - didn't look at the formatting changes.
Tree-SHA512: f83e593663a69182986325d9ba2b4b787b87896d6648973f4f802f191a2573201b9e7d7e10e69662ef1965fa63268845726ed1aa5742a2e38dcccf4aebc6a961
fa3669f72f fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883f5a addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa298971e6 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c633dc move-only: Move CAddrMan::Check to cpp file (MarcoFalke)
Pull request description:
Assert should only be used for program internal logic errors, not to sanitize external user input.
The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70fd9, thus won't need a backport.
Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.
For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes#22931.
Also,
Fixes#22503Fixes#22504Fixes#22519Closes#22498
Steps to test:
```
mkdir -p /tmp/test_235/regtest/
echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
```
Output before:
```
2021-09-10T11:28:37Z init message: Loading P2P addresses…
2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.
(program crashes)
```
Output after:
```
2021-09-10T11:26:00Z init message: Loading P2P addresses…
2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
(program exits)
```
ACKs for top commit:
naumenkogs:
ACK fa3669f72f
jnewbery:
Code review ACK fa3669f72f
vasild:
ACK fa3669f72f
Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6
f680d27155 test: use MiniWallet for make_utxo helper in feature_rbf.py (Sebastian Falbesoner)
0f27524602 test: scale amounts in test_doublespend_tree down by factor 10 (Sebastian Falbesoner)
d1e2481274 test: scale amounts in test_doublespend_chain down by factor 10 (Sebastian Falbesoner)
Pull request description:
This PR aims to further increase MiniWallet usage in the functional test feature_rbf.py by using it in the `make_utxo(...)` helper, which is the only part that needs a wallet for most sub-tests. In order to do that, the amounts for the utxos have to be scaled down in two sub-tests first (`test_doublespend_chain` and `test_doublespend_tree`, see first two commits), since we need amounts passed to `make_utxo` than can be funded by only one input. For creating UTXOs with a value of 50 BTC, we'd need to implement a method for consolidating multiple utxos into one first, which seems to be overkill.
Note that after this PR's change, there is only one sub-test left (`test_rpc`) that needs the wallet compiled into bitcoind.
ACKs for top commit:
MarcoFalke:
review ACK f680d27155🦐
Tree-SHA512: 46c8c245086a9e79855c4ede2f8f412333cf2658136805196b203b3567c89398d77fcb80715c0bb72fdc84331cc67544b2fdc259193a3adcb2fc36e147c26fce
cdaab90662 Add test for addrman consistency check on restart with asmap (Jon Atack)
869f136816 Add test for rpc addpeeraddress with "tried" argument (Jon Atack)
ef242f5213 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack)
Pull request description:
This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.
PR #22697 introduced a reproducible bug in commit 181a1207 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used.
The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before.
Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.
```
addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
```
How to reproduce:
- `git checkout 181a1207`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled
- restart bitcoind
- bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()`
How to test this pull:
- `git checkout 181a1207`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed.
```
ACKs for top commit:
jnewbery:
utACK cdaab90662
mzumsande:
re-ACK cdaab90662 (based on code review of diff to d586817)
vasild:
ACK cdaab90662
Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399
faa81f9486 test: Add addrman deserialization error tests (MarcoFalke)
Pull request description:
Add missing test coverage
ACKs for top commit:
jonatack:
Light code review ACK faa81f9486 and ran the test
Tree-SHA512: 8b254ba912c83473125faaf7df02a33a99840b40460bdce1486991a01de9ba6371c053354318f09b69fdc18c823bca3f2f7d341db0f8950e22d8435acbaa9cf5
fa7e3f1fc1 test: Replace MiniWallet scan_blocks with rescan_utxos (MarcoFalke)
Pull request description:
This avoids having to fiddle with the `start` and `num` parameters and instead use the `scantxoutset` RPC functionality via `rescan_utxos`.
ACKs for top commit:
Shubhankar-Gambhir:
ACK fa7e3f1, all tests were succesfull
theStack:
re-ACK fa7e3f1fc1
Tree-SHA512: 6f47d2acac9f180b2b0f8f04797e74ecb1fc180f6b164c67813a3a1f97acea54baed74e5e0a3512e3babf76b105c09e1ba4cad818c83c7cb2beb7377b4c96954
9bdda50151 Enable TLS in links in documentation (Jeremy Rand)
Pull request description:
This PR enables TLS in several documentation links, which improves security.
ACKs for top commit:
fanquake:
ACK 9bdda50151
Tree-SHA512: 9d04d8771a9daf3c3b9914ff324e2eabfdf3ff5ae7f7dc92b84a1f3527010ceb860e73873a8f24d6051763eb472d9ea324ccbd6129a40318a520ca88c05f0586
7c3712fa32 cli: Display all proxies in -getinfo (klementtan)
Pull request description:
**Changes**: Display all proxies in `-getinfo`
**Motivation**:
* Currently `-getinfo` only return the proxy of the first network in `getnetworkinfo`.
* This PR will display all unique proxies in `getnetworkinfo` as suggested in https://github.com/bitcoin/bitcoin/issues/17314#issue-514543978
>List all proxies, at least if they're different from the IPv4 one
**Testing**:
You can verify this change by starting bitcoind with
```shell
./src/bitcoind -signet --proxy=127.0.0.1:9050 --i2psam=127.0.0.1:7656
```
Execute `-getinfo`
```shell
./src/bitcoin-cli -signet -getinfo
```
ACKs for top commit:
laanwj:
Tested ACK 7c3712fa32
prayank23:
utACK 7c3712fa32
Tree-SHA512: 9eae97866220227f30ca4585f52799fa66fc1135047d869c4aabe598aee1a9414cb9e1c4a8d19165e52d65005f3c6d4bcc37463ace0ddb44389dfbcd4ca74095
Due to evil floating-point arithmetic, the creation of one of the
transaction outputs in feature_coinstatsindex.py leads to it's nValue
being off by one satoshi: the Python expression `int(21.99 * COIN)`
doesn't yield 2199000000 as expected, but 2198999999.
This makes the test more confusing than necessary (w.r.t. the expected
`gettxoutsetinfo` values), and could also cause problems if the value
is ever changed. Fix by using a `Decimal` type for specifying the
value in BTC, rather than using a bare floating-point.
357f0c7233 ci: Enable more functional tests on Windows MSVC task (Hennadii Stepanov)
f55932678f qa: Fix "RuntimeError: Event loop is closed" on Windows (Hennadii Stepanov)
Pull request description:
On master (2161a05855), running functional tests that use the P2P interface ends with an error:
```
RuntimeError: Event loop is closed
```
This PR fixes this bug, and enables more functional tests on Windows MSVC CI task.
More details about bugfix:
- [What’s New In Python 3.7](https://docs.python.org/3/whatsnew/3.7.html#asyncio)
- https://bugs.python.org/issue33792
- actual [change](https://docs.python.org/3.8/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy) done in Python 3.8
Excluded tests, that are listed in the `EXCLUDE_TESTS` environment variable, need more thorough investigation to be enabled.
ACKs for top commit:
MarcoFalke:
review ACK 357f0c7233🌆
Tree-SHA512: d0ba85be81d55c934959ce7402a9c726598125e9751a1de179d16759d0e8b8a915de879c3a62c12d3564c5e0d9649ebd86963744449626efaa42d9eaa99ad3d0
fa66a7d732 p2p: Rename fBlocksOnly, Add test (MarcoFalke)
fac66d0a39 test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method (MarcoFalke)
Pull request description:
`fBlocksOnly` has several issues:
* The name is confusing
* It is untested
Fix both.
ACKs for top commit:
laanwj:
Code review ACK fa66a7d732
Tree-SHA512: 4218f455eeb37297f74603d7d44895288605844ae828a40dfb7a70215f1a058ac5ad945a22732f5ebcad3ad375d54ba360bea69ea79639a30d4c88b042448f0f
fad4f44645 test: Set peertimeout in write_config (MarcoFalke)
Pull request description:
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
ACKs for top commit:
laanwj:
Concept and code review ACK fad4f44645
Tree-SHA512: 00c742571c0524c1b3f55e0217433ef7aa2dccccc12650caab98b4cf9231669f37fc589c7475f28d5725ffe2436c76205920eaece4a47fd27dc8872421a48e5c
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
PR #22697 introduced a reproducible issue in commit 181a1207 that causes the
addrman tried table to fail consistency checks and significantly lose peer
entries when the `-asmap` configuration option is used.
The issue occurs on bitcoind restart due to an initialization order change
in `src/init.cpp` in that commit whereby CAddrman asmap is set after
deserializing `peers.dat`, rather than before.
Issue reported on the `#bitcoin-core-dev` IRC channel starting at
https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.
```
addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17
bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
```
How to reproduce:
- `git checkout 181a1207` and recompile
- launch bitcoind with `-asmap` and `-checkaddrman=1` config options
- restart bitcoind
- bitcoind aborts on second call to `CAddrMan::Check()`
This commit adds a regression test to reproduce the case; it passes or fails
with the same error.
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
The helper `send_large_txs` in its current interface has a fee_rate
parameter, implying that it would create a transaction with exactly that
rate. Unfortunately, this fee rate is only passed to MiniWallet's
`create_self_transfer` method, which can't know that we append several
tx outputs after, increasing the tx's vsize and decreasing it's fee rate
accordingly.
In our case, the fee rate is off by several orders of magnitude, as the
tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the
value passed to this function is neither really a fee rate nor an
absolute fee, but something in-between, which is very confusing.
Clarify the interface by passing an absolute fee that is deducted in the end
(and verified, via testmempoolaccept) and also describe how we come up with the
value passed.
08634e82c6 fix typos in logging messages (ShubhamPalriwala)
d447ded6ba replace: self.nodes[0] with node (ShubhamPalriwala)
dddca3899c test: use MiniWallet in mempool_limit.py (ShubhamPalriwala)
Pull request description:
This is a PR proposed in #20078
This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with --disable-wallet.
It also includes changes in wallet.py in the form of a new method, `create_large_transactions()` for the MiniWallet to create large transactions.
Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up.
To test this PR locally, compile and build bitcoin-core without the wallet and run:
```
$ test/functional/mempool_limit.py
```
ACKs for top commit:
amitiuttarwar:
ACK 08634e8, only git changes since last push (and one new line).
Zero-1729:
ACK 08634e82c6🧉
Tree-SHA512: 0f744ad26bf7a5a784aac1ed5077b59c95a36d1ff3ad0087ffd10ac8d5979f7362c63c20c2ce2bfa650fda02dfbcd60b1fceee049a2465c8d221cce51c20369f
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
The new name describes better what the bool does and also limits the confusion of the three different concepts:
* fBlocksOnly (This bool to skip tx invs)
* -blocksonly (A setting to ignore incoming txs)
* block-relay-only (A connection type in the block-relay-only P2P graph)
fa55c3dc1b Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2ccfd8 Inline ReadPeerAddresses (MarcoFalke)
fa5aeec80c Move LoadAddrman from init to addrdb (MarcoFalke)
Pull request description:
peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
* The user provided the database, but picked the wrong one.
* A future version of Bitcoin Core wrote the file and it can't be read.
* The file was corrupted by a logic bug in Bitcoin Core.
* The file was corrupted by a disk failure.
ACKs for top commit:
jonatack:
Code review re-ACK fa55c3dc1b per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
prayank23:
tACK fa55c3dc1b
vasild:
ACK fa55c3dc1b
Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196
fa676dbac8 test: pep-8 whitespace (MarcoFalke)
faed284eab test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)
Pull request description:
Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):
```
node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
```
Fix that by skipping `InactivityCheck` via a large `-peertimeout`.
ACKs for top commit:
fanquake:
ACK fa676dbac8
Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
fa7db1cbf7 [test] checks descendants limtis for second generation Package descendants (ritickgoenka)
Pull request description:
This PR adds a new functional test to test the new descendant limits for packages that were proposed in #21800.
```
+----------------------+
| |
| M1 |
| ^ ^ |
| M2 ^ |
| . ^ |
| . ^ |
| . ^ |
| . ^ |
| M24 ^ |
| ^ |
| P1 |
| ^ |
| P2 |
| |
+----------------------+
```
This test is for checking a transaction to fail its descendant count limits because of a combination of mempool descendants, package direct descendants, and package indirect descendants.
In this test, P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when combined P2 has M1 as an ancestor and M1 exceeds descendant_limits (23 in-mempool descendants + 2 in-package descendants, a total of 26 including itself)
ACKs for top commit:
ryanofsky:
Code review ACK fa7db1cbf7. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested
glozow:
ACK fa7db1cbf7
jnewbery:
ACK fa7db1cbf7
Tree-SHA512: d1eb993550ac8ce31cbe42e17c6522a213ede66970d5d9391f31a116477ab5889fefa6ff2df6ceadd63a28c1be1ad893b0e8e449247e9ade2ca61dc636508d68
e6998838e5 doc: Add IPv6 address to zmq example (nthumann)
8abe5703a9 test: Add IPv6 test to zmq (nthumann)
ded449b726 zmq: Enable IPv6 on listening socket (nthumann)
Pull request description:
This PR adds support for listening on IPv6 addresses with bitcoinds ZMQ interface, just like the RPC server.
Currently, it is not possible to specify an IPv6 address, as the `ZMQ_IPV6` [socket option](http://api.zeromq.org/master:zmq-setsockopt#toc27) is not set and therefore the ZMQ initialization fails, if one does so. The absence of this option has also been noted [here](https://github.com/bitcoin/bitcoin/issues/15198#issuecomment-617378512).
With this PR one can e.g. set `-zmqpubhashblock=tcp://[::1]:28333` to listen on the IPv6 loopback address.
ACKs for top commit:
laanwj:
Code review ACK e6998838e5
theStack:
Tested ACK e6998838e5🌱
Tree-SHA512: 43c3043d8d5c79794d475926259c1be975b694db4fcc1f7750a9a28e242f0fa1b531735a63ea5777498003aa5834f6243f39742d0f3941f2f37593d0c7890700
fa0b916971 scripted-diff: Use generate* from TestFramework (MarcoFalke)
Pull request description:
This is needed for #22567.
By using the newly added `generate*` member functions of the test framework, it paves the way to make it easier to implicitly call `sync_all` after block generation to avoid intermittent issues.
ACKs for top commit:
jonatack:
ACK fa0b916971
Tree-SHA512: e74a324b60250a87c08847cdfd7b6ce3e1d89b891659fd168f6dd7dc0aa718d0edd28285374a613f462f34f4ef8e12c90ad44fb58721c91b2ea691406ad22c2a
fab0b55cf0 addrman: Fix format string in deserialize error (MarcoFalke)
facce4ca44 test: Remove useless overwrite (MarcoFalke)
Pull request description:
The format string is evaluated differently on modern compilers (clang 10 and later, as well as gcc 10 and later).
Work around the behaviour change in compilers by pinning the underlying type of the format arguments.
Can be tested by observing a failing test when running against master compiled with clang 10 or gcc 10 (or later).
ACKs for top commit:
jonatack:
ACK fab0b55cf0 verified the test fails on master as expected only at line 61 (assertion fixed by the code change); the last two test additions pass as expected
mzumsande:
ACK fab0b55cf0
Tree-SHA512: 07462901435107f3bc79098fd7d06446bfe8fe065fffdd35adfcba8f1dd3c499575006557afe7bc74b79d690c5ef7b58e3e031e908161be5529cf237e3b30609
7e69873283 sync: remove DEBUG_LOCKCONTENTION preprocessor directives (Jon Atack)
9b08006bc5 log, sync: improve lock contention logging and add time duration (Jon Atack)
3f4c6b87f1 log, timer: add timing macro in usec LOG_TIME_MICROS_WITH_CATEGORY (Jon Atack)
b7a17444e0 log, sync: add LOCK logging category, apply it to lock contention (Jon Atack)
Pull request description:
To enable lock contention logging, `DEBUG_LOCKCONTENTION` has to be defined at compilation. Once built, the logging is not limited to a category and is high frequency, verbose and in all-caps. With these factors combined, it seems likely to be rarely used.
This patch:
- adds a `lock` logging category
- adds a timing macro in microseconds, `LOG_TIME_MICROS_WITH_CATEGORY`
- updates `BCLog::LogMsg()` to omit irrelevant decimals for microseconds and skip unneeded code and math
- improves the lock contention logging, drops the all-caps, and displays the duration in microseconds
- removes the conditional compilation directives
- allows lock contentions to be logged on startup with `-debug=lock` or at run time with `bitcoin-cli logging '["lock"]'`
```
$ bitcoind -signet -debug=lock
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 completed (4μs)
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 completed (4μs)
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 completed (20μs)
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 started
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 completed (3μs)
$ bitcoin-cli -signet logging
"lock": true,
$ bitcoin-cli -signet logging [] '["lock"]'
"lock": false,
$ bitcoin-cli -signet logging '["lock"]'
"lock": true,
```
I've tested this with Clang 13 and GCC 10.2.1, on Debian, with and without `--enable-debug`.
ACKs for top commit:
hebasto:
re-ACK 7e69873283, added a contention duration to the log message since my [previous](https://github.com/bitcoin/bitcoin/pull/22736#pullrequestreview-743764606) review.
theStack:
re-ACK 7e69873283🔏⏲️
Tree-SHA512: c4b5eb88d3a2c051acaa842b3055ce30efde1f114f61da6e55fcaa27476c1c33a60bc419f7f5ccda532e1bdbe70815222ec2b2b6d9226f29c8e94e598aacfee7
wallet_multisig_descriptor_psbt.py is refactored in this commit. While
behavior doesn't change we do cleanup the way wallets are accessed
throughout the test as this is done a lot for the various signers
and their multisigs. We also get rid of some shallow methods and
instead inline them for improved readability.
descriptors.md is improved to be more explicit about which wallet
(ie the signer or multisig) is required for each step.
3633b667ff Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b4b4 Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d2ad tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f13e tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2861 tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfbe44 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a484d tests: rpc_fundrawtx lock to UTXO types (Andrew Chow)
Pull request description:
To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared.
ACKs for top commit:
glozow:
reACK 3633b66 via `git range-diff 981b9d1...3633b66`, thanks for taking the suggestions
laanwj:
Concept and code review ACK 3633b667ff
Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92
fa01f22e6e test: Add missing re.escape() to feature_addrman test (MarcoFalke)
Pull request description:
Needed to run the test on windows
ACKs for top commit:
laanwj:
Code review ACK fa01f22e6e
hebasto:
ACK fa01f22e6e, passed 2 consequential runs in my [personal CI](https://cirrus-ci.com/task/6522304080379904).
Tree-SHA512: d7ca4fb882cc6693989ddf6fc092db3259a0619cb8f87293c588484b9c62e6755e9fb1bb2c1ab85fcc8f0349d9bc155ba515e16674c0f6f56236e7fbb14655a8
Instead of relying on coin selection to deterministically choose
the correct inputs to use, just specify them explicitly and use
the raw transaction RPCs.
Don't assume that specific inputs are going to be used when they aren't
specified explicitly.
Also fixes a bug in the include_unsafe test where after the inputs
confirm, include_unsafe should be set to False rather than True.
For the test that checks that there is no error when change is
unavailable but change is also not needed, use specific UTXOs so that
SRD does not cause this to fail when it chooses random inputs.
For some of the tests within rpc_fundrawtx, there is the expectation
that two independent calls to coin selection RPCs will use the same type
of UTXO. This is not necessarily guaranteed, so to make sure it is, use
lockunspent prior to those tests.
b11a195ef4 refactor: Detach wallet transaction methods (followup for move-only) (Russell Yanofsky)
Pull request description:
This makes `CWallet` and `CWalletTx` methods in `spend.cpp` and `receive.cpp` files into standalone functions.
It's a followup to [#21207 MOVEONLY: CWallet transaction code out of wallet.cpp/.h](https://github.com/bitcoin/bitcoin/pull/21207), which moved code from `wallet.cpp` to new `spend.cpp` and `receive.cpp` files.
There are no changes in behavior. This is just making methods into functions and removing circular dependencies created by #21207. There are no comment or documentation changes, either. Removed comments from `transaction.h` are just migrated to `spend.h`, `receive.h`, and `wallet.h`.
---
This commit was split off from #21206 so there are a few earlier review comments there
ACKs for top commit:
achow101:
ACK b11a195ef4
Sjors:
utACK b11a195ef4
meshcollider:
light ACK b11a195ef4
Tree-SHA512: 75ce818d3f03b728b14b12e2d21bd20b7be73978601989cb37ff98254393300d1bb7823281449cd3d9e40756d67d42bd9a46bbdafd2e8baa95aaf2cb1c84549f
fa2e9de59f test: Check that non-signaling BIP125 tx can be replaced via parent (MarcoFalke)
Pull request description:
While `optout_child_tx` in the `test_no_inherited_signaling` test is reported as "bip125-replaceable", it is not *directly* replaceable. For example by bumping the fee of `optout_child_tx`. However, it is still replaceable *indirectly* via it's BIP-125 signalling parent.
Clarify this by extending the test.
ACKs for top commit:
mjdietzx:
Tested ACK fa2e9de59f
josibake:
ACK fa2e9de59f
Tree-SHA512: b3608beae743dcb6152df4d2cfe1c0af6b4404ba3837f73e1d1431bd7c637f0c7fab0379aaab2218d5cd63e71070a079c0595ec031056058e8d3c933c2bae0a9
387355bb94 test, refactor: rpc_rawtransaction PEP8 (Jon Atack)
7d5cec2e49 refactor: separate the rpc_rawtransaction tests into functions (Jon Atack)
409779df95 move-only: regroup similar rpc_rawtransaction tests together (Jon Atack)
d861040dd2 test: remove no longer needed (ASCII art) comments (Jon Atack)
14398b30d6 test: add and harmonize getrawtransaction logging (Jon Atack)
85d8869cf8 test: run 2nd getrawtransaction section with/without -txindex (Jon Atack)
0097740773 refactor: txid to constant in rpc_rawtransaction to isolate tests (Jon Atack)
8c19d1329f refactor: dedup/reorg createrawtransaction sequence number tests (Jon Atack)
7f073594c9 Test src/node/transaction::GetTransaction() without -txindex (Jon Atack)
Pull request description:
Following up on https://github.com/bitcoin/bitcoin/pull/22383#pullrequestreview-698583510, this pull adds missing `src/node/transaction::GetTransaction()` test coverage for combinations of `-txindex` and `blockhash` and does some refactoring of the test file.
ACKs for top commit:
mjdietzx:
reACK 387355bb94
josibake:
reACK 387355bb94
MarcoFalke:
Approach ACK 387355bb94🔆
Tree-SHA512: b47c4ff87d69c61434e5729c954b338bc13744eddaba0879ca9f5f42243ba2cb4640d94c5f74de9f2735a8bf5e66b3d1c3bd3b7c26cd7324da7d3270ce87c6fd
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee0579 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e Add waste metric calculation function (Andrew Chow)
935b3ddf72 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f90 tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b4746 Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee0579
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
faf7e485e9 Set regtest.BIP65Height = 111 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.
ACKs for top commit:
theStack:
re-ACK faf7e485e9📍
Zero-1729:
re-ACK faf7e485e9
kristapsk:
ACK faf7e485e9
Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
7720d4f650 test: fix failure in feature_nulldummy.py on single-core machines (Sebastian Falbesoner)
646b3885f7 test: refactor: use named args for block_submit in feature_nulldummy.py (Sebastian Falbesoner)
Pull request description:
On single-core machines, executing the test `feature_nulldummy.py` results in the following assertion error:
```
...
2021-08-18T15:37:58.805000Z TestFramework (INFO): Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation
2021-08-18T15:37:58.814000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "[...]/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "[...]/test/functional/feature_nulldummy.py", line 107, in run_test
self.block_submit(self.nodes[0], [test4tx], accept=False)
File "[...]/test/functional/feature_nulldummy.py", line 134, in block_submit
assert_equal(None if accept else 'block-validation-failed', node.submitblock(block.serialize().hex()))
File "[...]/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(block-validation-failed == non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero))
2021-08-18T15:37:58.866000Z TestFramework (INFO): Stopping nodes
...
```
There are hardly any single-core machines around anymore, but the behaviour can be reproduced on a multi-core machine by patching the function `GetNumCores()` to return 1 on the master branch and running `feature_nulldummy.py`:
```diff
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 30d410381..149b512fc 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -1338,7 +1338,7 @@ bool SetupNetworking()
int GetNumCores()
{
- return std:🧵:hardware_concurrency();
+ return 1;
}
```
As solution, parallel script verification is disabled (`-par=1`) and the exact reject reason is checked, which also increases the precision of the test (the possibility that the block is rejected because of another unintended reason is ruled out). See also related PR #22711 which applies the same approach for the p2p segwit test. The PR also includes a refactoring commit which changes the calls to `self.block_submit()` to use named arguments and removes the default value for parameter `accept` (i.e. explicitely passing `accept=...` is mandatory), with the aim to increase the test readability.
ACKs for top commit:
josibake:
ACK 7720d4f650
Saviour1001:
Tested ACK <code>[7720d4f](7720d4f650)</code>
Tree-SHA512: 8a31ebab3e2ab38e555d7a23139b3324a134a0dedc5b879a2419348ae858323882dbbfcbbf88b68e4f8d7eea8cfe43ee19da1d0d2a36c93ae7878c4980cac31d
0d9fdd329e test, doc: refer to the correct variable names in p2p_invalid_tx.py (aitorjs)
Pull request description:
_tx_orphan_no_fee_ and _tx_orphan_invalid_ don't exist as transactions.
Have been replaced by _tx_orphan_2_no_fee_ and _tx_orphan_2_invalid_ respectively.
**Motivation**: Comments are more accurate and easy understandable under the tests context (I think).
ACKs for top commit:
kristapsk:
utACK 0d9fdd329e
theStack:
ACK 0d9fdd329e📃
Tree-SHA512: a4cafd931e51fe2a67085e10e9c61178c864c14982664d112b76327e040af08cd1de04eca4a8ae980fad57ba7078017ce02fc60e7658f38380e8172c2ae28b77
fab2e23b57 Use generate* from TestFramework (MarcoFalke)
faf7e92804 test: Add generate* calls to test framework (MarcoFalke)
Pull request description:
This is needed for #22567.
By making the calls to `generate*` member function of the test framework, it paves the way to make it easier to implicitly call the `sync_all` member function.
ACKs for top commit:
jnewbery:
utACK fab2e23b57
Tree-SHA512: 7a7be6be71f0602119689df45d63a1adec309f323eac2330ee0f200676001afe825605859bd02c6a8a8dcf85d925dc1bc37370ef1ceb8ad1d85a66eec0dbfff9
On single-core machines, executing the test feature_nulldummy.py results in
the following assertion error:
...
2021-08-18T15:37:58.805000Z TestFramework (INFO): Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation
2021-08-18T15:37:58.814000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "[...]/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "[...]/test/functional/feature_nulldummy.py", line 107, in run_test
self.block_submit(self.nodes[0], [test4tx], accept=False)
File "[...]/test/functional/feature_nulldummy.py", line 134, in block_submit
assert_equal(None if accept else 'block-validation-failed', node.submitblock(block.serialize().hex()))
File "[...]/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(block-validation-failed == non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero))
2021-08-18T15:37:58.866000Z TestFramework (INFO): Stopping nodes
...
The behaviour can be reproduced on a multi-core machine by simply changing the
function GetNumCores() (in src/util/system.cpp) to return 1:
int GetNumCores()
{
return 1;
}
127b4608e9 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9
Zero-1729:
tACK 127b4608e9
theStack:
Tested ACK 127b4608e9
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
a3b559c970 test: added test for disabled wallet (Shubhankar Gambhir)
Pull request description:
This PR enables a part of the non-wallet functional test (rpc_signmessage.py) to be run even with the Bitcoin Core wallet disabled, it is inspired by #20078.
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled that provides extra test which was not performed earlier.
* we need bitcoincore wallet to run rpc_signmessage.py, but it is olny required for signing messages with address and not for signing messages wih private key, so latter one can be in a seperate test which can run without wallet
* verifying message doesn't require wallet, so it can be used in both tests without any problem
* 2 tests are named as wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py to provide clarity of what they are testing.
ACKs for top commit:
vasild:
ACK a3b559c970
theStack:
Code-review ACK a3b559c970
Tree-SHA512: 1bfca3baf3123a02f0a2389e55e141d64430c3bed40ff5a5fb97ef2c66e2853c46e4b2dff62b948eb94dc574cb89d061769330f0535e2d5d1be76b60101136ac
021daedfa1 refactor: replace remaining binascii method calls (Zero-1729)
Pull request description:
This PR removes the remaining `binascii` method calls outside `test/functional` and `test_framework`, as pointed out here https://github.com/bitcoin/bitcoin/pull/22619#pullrequestreview-722153458.
Follow-up to #22593 and #22619Closes#22605
ACKs for top commit:
josibake:
re-ACK 021daedfa1
theStack:
re-ACK 021daedfa1
Tree-SHA512: 2ae9fee8917112c91a5406f219ca70f24cd8902b903db5a61fc2de85ad640d669a772f5c05970be0fcee6ef1cdd32fae2ca5d1ec6dc9798b43352c8160ddde6f
47c48b5f35 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b1 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8 test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
e2ff385e13 test: check for invalid `-prune` parameters (Sebastian Falbesoner)
Pull request description:
This small PR adds missing test coverage for invalid `-prune` parameter values / combinations:
77e23ca945/src/init.cpp (L926-L928)77e23ca945/src/init.cpp (L935-L937)77e23ca945/src/init.cpp (L844-L849)
Not sure if the tests fit into `feature_config_args.py` or should rather be moved into `feature_pruning.py`; the latter though seems to be run less often due to being very memory-hungry.
ACKs for top commit:
laanwj:
Code review ACK e2ff385e13
Tree-SHA512: bb0db98090058ecac9f8a01301634e9dba9a65fd56b6a0b770f88da28c4f01e240e22b1225f0d231e28bdd4b5b51bff0e6853cccc46ed0190e91b84f7954a9db
92885c4f69 test: Test for ApproximateBestSubset edge case with too little fees (Andrew Chow)
d9262324e8 wallet: Assert that enough was selected to cover the fees (Andrew Chow)
2de222c401 wallet: Use GetSelectionAmount for target value calculations (Andrew Chow)
Pull request description:
The `m_value` used for the target calculation in `ApproximateBestSubset` is incorrect, it should be `GetSelectionAmount`. This causes a bug that is only apparent when the minimum relay fee is set to be very high.
A test case is added for this, in addition to an assert in `CreateTransactionInternal` that would have also caught this issue if someone were able to hit the edge case.
Fixes#22670
ACKs for top commit:
instagibbs:
utACK 92885c4f69
Tree-SHA512: bd61fa61ffb60873e097737eebea3afe8a42296ba429de9038b3a4706763b34de9409de6cdbab21ff7f51f4787b503f840873182d9c4a1d6e12a54b017953547
5fe8100ff3 Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23faba6f Add a new RPC command: restorewallet (lsilva01)
Pull request description:
As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.
This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.
To test:
First create a backup file:
`$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`
Then restore it in another wallet:
`$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`
ACKs for top commit:
achow101:
re-ACK 5fe8100ff3
prayank23:
tACK 5fe8100ff3
meshcollider:
utACK 5fe8100ff3
Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
d930c7f5b0 p2p, rpc, test: address rate-limiting follow-ups (Jon Atack)
Pull request description:
Incorporates review feedback in #22387.
Edit, could be considered separately: should a release note (or two) be added for 22.0? e.g. the new getpeerinfo fields in `Updated RPCs` and the rate-limiting itself in `P2P and network changes`.
ACKs for top commit:
MarcoFalke:
review ACK d930c7f5b0
theStack:
re-ACK d930c7f5b0🌮
Zero-1729:
crACK d930c7f
Tree-SHA512: b2101cad87f59c238603f38bd8e8df7a4d48929794e4de9e0e0ff2afa935a68475c2d369aa669d124a0bec2f50280fb47e8b980bde6ad812db08cf67b71c066a
ApproximateBestSubset had an edge case (due to not using
GetSelectionAmount) where it was possible for it to return success but
fail to select enough to cover transaction fees. A test is added that
could trigger this failure prior to the fix being implemented.
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and
rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled.
fafe896a0b test: Set regtest.BIP66Height = 102 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.
ACKs for top commit:
GeneFerneau:
Concept + code review ACK [fafe896](fafe896a0b)
0xB10C:
crACK fafe896a0b
laanwj:
ACK fafe896a0b
Zero-1729:
tACK fafe896
kristapsk:
ACK fafe896a0b. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
theStack:
Tested ACK fafe896a0b
hg333:
tACK fafe896a0b
Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
accf3d5868 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7 [test] helper function to increase transaction weight (glozow)
f8253d69d6 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d3 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa13 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aa misc package validation doc improvements (glozow)
Pull request description:
This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).
Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.
#### Motivation
It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:
- We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
- We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.
ACKs for top commit:
JeremyRubin:
utACK accf3d5
ariard:
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
bb822a7af8 wallet, rpc: add listdescriptors private option (S3RK)
Pull request description:
Rationale: make it possible to backup your wallet with `listdescriptors` command
* The default behaviour is still to show public version
* For private version only the root xprv is returned
Example use-case:
```
> bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
> bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt
> bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
> bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
```
In case of watch-only wallet without private keys there will be following output:
```
error code: -4
error message:
Can't get descriptor string.
```
ACKs for top commit:
achow101:
re-ACK bb822a7af8
Rspigler:
tACK bb822a7af8
jonatack:
ACK bb822a7af8 per `git diff 2854ddc bb822a7`
prayank23:
tACK bb822a7af8
meshcollider:
Code review ACK bb822a7af8
Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
5a1bef60a0 test: refactor: remove binascii from test_framework (Zero-1729)
Pull request description:
This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`, and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.
Additionally, certain changes made are based on the following assumption:
```
bytes.hex(data) == binascii.hexlify(data).decode()
bytes.hex(data).encode() == binascii.hexlify(data)
```
Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.
closes#22605
ACKs for top commit:
theStack:
Code-review ACK 5a1bef60a0🔢
Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
222290f543 test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f543
jonatack:
ACK 222290f543 tested and reviewed rebased to current master 5e213822f8
theStack:
Tested ACK 222290f543
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
82b6f89819 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64 [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed56 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bd [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2 [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a9 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c08719778 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)
Pull request description:
This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).
The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.
The tests include:
* parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
* the delay before querying DNS seeds depending on how many addresses are in the addrman
* the behavior of `-forcednsseed`
* skipping DNS querying if we have outbound full relay connections & not block-relay-only connections
Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽
ACKs for top commit:
mzumsande:
ACK 82b6f89819
jnewbery:
reACK 82b6f89819
Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
3f7250b328 [test] Use the new endpoint to improve tests (Amiti Uttarwar)
3893da06db [RPC] Add field to getpeerinfo to indicate if addr relay is enabled (Amiti Uttarwar)
0980ca78cd [test] Test that we intentionally select addr relay peers. (Amiti Uttarwar)
c061599e40 [net_processing] Remove RelayAddrsWithPeer function (Amiti Uttarwar)
201e496481 [net_processing] Introduce new field to indicate if addr relay is enabled (Amiti Uttarwar)
1d1ef2db7e [net_processing] Defer initializing m_addr_known (Amiti Uttarwar)
6653fa3328 [test] Update p2p_addr_relay test to prepare (Amiti Uttarwar)
2fcaec7bbb [net_processing] Introduce SetupAddressRelay (Amiti Uttarwar)
Pull request description:
This PR builds on the test refactors extracted into #22306 (first 5 commits).
This PR aims to reduce addr blackholes. When we receive an `addr` message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn't relay would effectively "blackhole" the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.
This implementation defers initialization of `m_addr_known` until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their `version` message. For inbound peers, we initialize the filter if/when we get an addr related message (`ADDR`, `ADDRV2`, `GETADDR`). We do NOT initialize the filter based on a `SENDADDRV2` message.
To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:
- Posted to the [mailing list](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018784.html)
- Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in https://github.com/bitcoin/bitcoin/pull/21528#issuecomment-809906430. Many have confirmed that this change would not be problematic.
- Raised as topic during [bitcoin-core-dev meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-03-25.html#l-954)
- Raised as topic during [bitcoin p2p meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-04-20.html#l-439)
ACKs for top commit:
jnewbery:
reACK 3f7250b328
glozow:
ACK 3f7250b328
ajtowns:
utACK 3f7250b328
Tree-SHA512: 29069282af684c1cd37d107c395fdd432dcccb11626f3c2dabfe92fdc4c85e74c7c4056fbdfa88017fec240506639b72ac6c311f8ce7c583112eb15f47e421af
607076d01b test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c74ed test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe133 test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)
Pull request description:
This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous).
Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.
ACKs for top commit:
MarcoFalke:
review ACK 607076d01b🚴
Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
8a2b58db9e test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)
Pull request description:
This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program) is defined as follows:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". **The following byte vector pushed is called the "witness program".**
In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](https://github.com/bitcoin/bitcoin/pull/22363#discussion_r666794261)!
Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f31ee, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
This was fixed in PR https://github.com/bitcoin/bips/pull/416 later.
So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.
ACKs for top commit:
josibake:
tACK 8a2b58db9e
Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
When starting up with a populated addrman, ThreadDNSAddressSeed adds a delay
during which time the node may be able to connect to some peers. This commit
tests the delay changes based on the number of addresses in the addrman.
Test that passing conflicting parameters for the two causes a startup error.
This logic also impacts -connect, which soft sets -dnsseed, so add a test for
that too.
When a node is able to properly shutdown, it will persist its block-relay-only
connections to the addrman. On startup, it will attempt to reconnect to these
anchors. Since block-relay-only connections do not participate in ADDR relay,
succesful connections are insufficient to skip querying the DNS seeds.
This test fails prior to the changes in #22013.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This commit introduces a DNS seed to the regest chain params in order to add
coverage to the DNS querying logic.
The first test checks that we do not query DNS seeds if we are able to
succesfully connect to 2 outbound connections. Since we participate in ADDR
relay with those connections, including sending a GETADDR message during the
VERSION handshake, querying the DNS seeds is unnecessary.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
aa02c64540 test: use MiniWallet for simple doublespend test in feature_rbf.py (Sebastian Falbesoner)
a3f6397c73 test: feature_rbf.py: make MiniWallet instance available for all sub-tests (Sebastian Falbesoner)
84c874794c test: remove unneeded initialization code in feature_rbf.py (Sebastian Falbesoner)
Pull request description:
This PR's goal is to prepare the functional test `feature_rbf.py` for more MiniWallet usage. It first gets rid of unused initialization code (I guess that was needed at times when the nodes were still in IBD at the start of tests?), then makes the MiniWallet instance introduced in #22210 available for all sub-tests, and finally, uses that instance in the first sub-test `test_simple_doublespend`.
Note that the same idea of replacing the `make_utxo` calls with MiniWallet can be also applied to other sub-tests too; this just serves as a first proof-of-concept.
ACKs for top commit:
MarcoFalke:
re-ACK aa02c64540🌯
Tree-SHA512: 2902dd15d493d83b0790028c92d14fbd99ca05ace704c7011fb38261ce6517aeb810ef9f360fcb701d95887975b6a2911cfe538858d38fceb2c1c2a40afdbe6b
8ca51af1ec test: Disable automatic connections by default (Martin Zumsande)
Pull request description:
A node normally doesn't make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), `ThreadOpenConnections` will periodically try to connect to them, resulting in log entries such as:
`[opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs`
I don't think it's desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test.
This PR fixes this problem by disabling `ThreadOpenConnections` by adding `-connect=0` to the default args, and adding exceptions only when needed for the test to pass.
ACKs for top commit:
tryphe:
Concept ACK, light code review ACK 8ca51af1ec
Tree-SHA512: bcfb2de610e6c35a97a2bd7ad6267e968b1ac7529638d99276993cd5bc93ce9919d54e22d6dc84e1b02ecd626ab6554e201693552ea065c29794eece38c43f7d
ef5e9304cd test: update logging and docstring in rpc_blockchain.py (Jon Atack)
d548dc71e4 test: replace magic values by constants in rpc_blockchain.py (Jon Atack)
78c361086f test: assert on mediantime in getblockheader and getblockchaininfo (Jon Atack)
0a9129c588 test: assert on the value of getblockchaininfo#time (Jon Atack)
Pull request description:
Follow-up to #22407 improving test coverage per https://github.com/bitcoin/bitcoin/pull/22407#pullrequestreview-702077013.
ACKs for top commit:
tryphe:
untested ACK ef5e9304cd
Tree-SHA512: f746d56f430331bc6a2ea7ecd27b21b06275927966aacf1f1127d8d5fdfd930583cabe72e23df3adb2e005da904fc05dc573b8e5eaa2f86e0e193b89a17a5734
This test checks that we only relay addresses with inbound peers who have sent
us an addr related message. Uses a combination of GETADDR and ADDR to verify
when peers are eligible.
Use SetupAddressRelay to only initialize `m_addr_known` as needed. For outbound
peers, we initialize the filter before sending our self announcement (not
applicable for block-relay-only connections). For inbound peers, we initialize
the filter when we get an addr related message (ADDR, ADDRV2, GETADDR).
These changes intend to mitigate address blackholes. Since an inbound peer has
to send us an addr related message to become eligible as a candidate for addr
relay, this should reduce our likelihood of sending them self-announcements.
Use an init param to make clear whether a getaddr message should be sent when
the P2PConnection receives a version message. These changes are in preparation
for upcoming commits that modify the behavior of a bitcoind node and the test
framework.
fbeb8c43bc test: add type annotations to util.get_rpc_proxy (fanquake)
Pull request description:
Split out from #22092 while we address the functional test failure.
ACKs for top commit:
instagibbs:
ACK fbeb8c43bc
Tree-SHA512: 031ef8703202ae5271787719fc3fea8693574b2eb937ccf852875de95798d7fa3c39a8db7c91993d0c946b45d9b4d6de570bd1102e0344348784723bd84803a8
779e638ca9 coinstats: Add comments for new coinstatsindex values (Fabian Jahr)
5b3d4e724f Index: Improve logging in coinstatsindex (Fabian Jahr)
d4356d4e48 rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo (Fabian Jahr)
a5f6791139 rpc: Add missing gettxoutsetinfo help docs (Fabian Jahr)
01386bfd88 Index: Return early from failed coinstatsindex init (Fabian Jahr)
1e3842385b index: Use batch writing in coinstatsindex WriteBlock (Fabian Jahr)
fb65dde147 scripted-diff: Fix coinstats data member names (Fabian Jahr)
8ea8c927ac index: Avoid unnecessary type casts in coinstatsindex (Fabian Jahr)
Pull request description:
This is a collection of smaller follow-ups to #19521, addressing several post-merge review comments.
ACKs for top commit:
Sjors:
re-utACK 779e638ca9
jonatack:
re-ACK 779e638ca9 diff since last review involves doc changes only; rebased to current master and verified clean debug build/no silent conflicts, unit tests, and feature_coinstatsindex functional test
laanwj:
Code review ACK 779e638ca9
Talkless:
re-utACK 779e638ca9 after cosmetic changes.
Tree-SHA512: cb0d038d230c582d7fe3041c89b1e04d39971fab3739d540c609cf826754c6c513b12ded08ac92180aec7a9d7a70114ece50357bd1a902de4adaae9f30b8d699
a006d7d730 test: add logging to wallet_listtransactions (Sebastian Falbesoner)
47915b1187 test: remove unneeded/redundant code in wallet_listtransactions (Sebastian Falbesoner)
fb6c6a7938 test: speedup wallet_listtransactions by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
This PR improves the test `wallet_listtransactions.py` in three ways:
* speeds up runtime by a factor of 2-3x by using the good ol' immediate tx relay trick (`-whitelist=noban@127.0.0.1`)
* removes unneeded/redundant code
* adds log messages, mostly by turning comments into `self.log.info(...)` calls
ACKs for top commit:
jonatack:
ACK a006d7d730
kristapsk:
ACK a006d7d730
Tree-SHA512: a91a19f5ebc4d05f0b96c5419683c4c57ac0ef44b64eeb8dd550bd72296fd3a2857a3ba83f755fe4b0b3bd06439973f226070b5d0ce2dee58344dae78cb50290
This prevents the node from trying to connect to random IPs on the internet
while running the functional tests. Exceptions are added when required for
the test to pass.
Without this check, the tests would also pass if the CSV and
CLTV activation heights are not reached yet (e.g. if the .generate()
calls before are removed), as the operations OP_CSV and OP_CLTV
simply behave as NOPs.
Also fixes a comment in the sub-test `test_signing_with_cltv()`.
During initial sync after startup the gettxoutsetinfo RPC will still return an error while catching up. However, after the initial sync the index will not error immediately anymore when it's in the process of syncing to the tip while being called. Instead it will block until synced and then return the response.
a806647d26 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db7 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b82 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd64 [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)
Pull request description:
Builds on #21009 and makes progress on remaining items in #17862
Removing `RewindBlockIndex()` in #21009 allows the following:
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
mzumsande:
Code-Review ACK a806647d26
laanwj:
Code review ACK a806647d26, nice cleanup
jnewbery:
utACK a806647d26
theStack:
ACK a806647d26
Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e
2ebf2fe0e4 test: check for RPC error 'Transaction already in block chain' (-27) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the RPC error "Transaction already in block chain" (error code `RPC_VERIFY_ALREADY_IN_CHAIN` = `RPC_TRANSACTION_ALREADY_IN_CHAIN` = -27), which is thrown in the function `BroadcastTransaction` (src/node/transaction.cpp).
ACKs for top commit:
kristapsk:
ACK 2ebf2fe0e4 (ran linter, looked at changes and ran modified test and checked code in `src/node/transaction.cpp`)
darosior:
ACK 2ebf2fe0e4
Tree-SHA512: 8bfbd3ff3da0cb3b8745f69b8ca2377f85fa99f0270750840b60e6ae43b5645c5c59b236993e8b2ad0444ec4171484e4f1ee23fa7e81b79d4222bcb623666fa5
20edf4bcf6 rpc: Return block time in getblockchaininfo (João Barbosa)
Pull request description:
Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.
ACKs for top commit:
naumenkogs:
ACK 20edf4bcf6
theStack:
re-ACK 20edf4bcf6
0xB10C:
ACK 20edf4bcf6
kristapsk:
ACK 20edf4bcf6
Zero-1729:
re-ACK 20edf4bcf6
Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
9b85a5e2f7 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7🎰
ryanofsky:
Code review ACK 9b85a5e2f7. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
5a77abd4e6 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c036 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1ea [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)
Pull request description:
1. Only add a transaction to the unbroadcast set when it's added to the mempool
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added to the mempool.
2. Allow rebroadcast for same-txid-different-wtxid transactions
There is some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.
ACKs for top commit:
duncandean:
reACK 5a77abd
naumenkogs:
ACK 5a77abd4e6
theStack:
re-ACK 5a77abd4e6
lsilva01:
re-ACK 5a77abd4e6
Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
Adds a test for the condition which can trigger a lock order assertion.
Specifically, there must be an unconfirmed transaction in the mempool
which belongs to the wallet being loaded. This will establish the order
of cs_wallet -> cs_main -> cs_KeyStore. Then dumpwallet is called on
that wallet. Previously, this would have used a lock order of cs_wallet
-> cs_KeyStore -> cs_main, but this should be fixed now. The test
ensures that.
a4bcd687c9 Improve tests using statistics (John Newbery)
f424d601e1 Add logging and addr rate limiting statistics (Pieter Wuille)
b4ece8a1cd Functional tests for addr rate limiting (Pieter Wuille)
5648138f59 Randomize the order of addr processing (Pieter Wuille)
0d64b8f709 Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
ACKs for top commit:
laanwj:
ACK a4bcd687c9
vasild:
ACK a4bcd687c9
jnewbery:
ACK a4bcd687c9
jonatack:
ACK a4bcd687c9
Tree-SHA512: b757de76ad78a53035b622944c4213b29b3b55d3d98bf23585afa84bfba10808299d858649f92269a16abfa75eb4366ea047eae3216f7e2f6d3c455782a16bea
-> remove unneeded get-out-of IBD generate()
(The test framework already sets up the nodes to be out of IBD
in setup_nodes(), if setup_clean_chain is not set to True)
-> remove duplicate code line assigning an utxo
By whitelisting the peers via -whitelist, the inventory is transmissioned
immediately rather than on average every 5 seconds, speeding up the test by at
least a factor of two:
before:
$ time ./wallet_listtransactions.py
...
0m40.25s real 0m01.74s user 0m01.70s system
with this PR:
$ time ./wallet_listtransactions.py
...
0m14.93s real 0m01.68s user 0m01.87s system
This commit also moves the wallet_listtransactions tests into the < 30s group.
While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
7593b06bd1 test: ensure I2P addresses are relayed (Vasil Dimov)
e7468139a1 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d2a4 test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
86742811ce test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02708 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)
Pull request description:
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.
Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
ACKs for top commit:
jonatack:
ACK 7593b06bd1
naumenkogs:
ACK 7593b06bd1.
laanwj:
Code review ACK 7593b06bd1
kristapsk:
ACK 7593b06bd1. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.
Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
a3d6ec5bb5 test: move rpc_rawtransaction tests to < 30s group (Jon Atack)
5a1ed96077 test: whitelist rpc_rawtransaction peers to speed up tests (Jon Atack)
Pull request description:
Speed up the somewhat slow `rpc_rawtransaction.py` test by more than 3x (from 45-55 seconds to 15 seconds on a laptop running 2 x 2.5GHz).
ACKs for top commit:
mjdietzx:
ACK a3d6ec5bb5
kristapsk:
ACK a3d6ec5bb5
theStack:
ACK a3d6ec5bb5🐎
brunoerg:
tACK a3d6ec5bb5
Tree-SHA512: f1d105594c9b5b257a7096b631a6fa5aeb50e330a351f75c2d6ffa7dd73abdb6e1f596a78c16d204a9bac3fe506e0519f9ad96bb8477ab6424c8e18125ccb659
fa80e10d94 test: Add feature_taproot.py --previous_release (MarcoFalke)
85ccffa266 test: move releases download incantation to README (Sjors Provoost)
29d6b1da2a test: previous releases: add v0.20.1 (Sjors Provoost)
Pull request description:
Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.
ACKs for top commit:
Sjors:
tACK fa80e10
NelsonGaldeman:
tACK fa80e10d94
Tree-SHA512: 1a1feef823f08c05268759645a8974e1b2d39a024258f5e6acecbe25097aae3fa9302c27262978b40f1aa8e7b525b60c0047199010f2a5d6017dd6434b4066f0
4101ec9d2e doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c1 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075 test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738 net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091e net: distinguish default port per network (Vasil Dimov)
aeac3bce3e net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290c net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e
jonatack:
re-ACK 4101ec9d2e per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce31 net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce31
jonatack:
utACK 2feec3ce31 per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce31, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
b7a8cd9963 [test] submit same txid different wtxid as mempool tx (glozow)
fdb48163bf [validation] distinguish same txid different wtxid in mempool (glozow)
Pull request description:
On master, if you submit a transaction with the same txid but different witness to the mempool, it thinks the transactions are the same. Users submitting through `BroadcastTransaction()` (i.e. `sendrawtransaction` or the wallet) don't get notified that there's a different transaction in the mempool, although it doesn't crash. Users submitting through `testmempoolaccept()` will get a "txn-already-in-mempool" error.
This PR simply distinguishes between `txn-already-in-mempool` and `txn-same-nonwitness-data-in-mempool`, without handling them differently: `sendrawtransaction` still will not throw, but `testmempoolaccept` will give you a different error.
I believe the intention of #19645 is to allow full swaps of transactions that have different witnesses but identical nonwitness data. Returning a different error message + adding a test was suggested: https://github.com/bitcoin/bitcoin/pull/19645#issuecomment-705109193 so this is that PR.
ACKs for top commit:
naumenkogs:
ACK b7a8cd9963
jnewbery:
Code review ACK b7a8cd9963
theStack:
Code-review ACK b7a8cd9963
darosior:
re-utACK b7a8cd9963
Tree-SHA512: 9c6591edaf8727ba5b4675977adb8cbdef7288584003b6cd659828032dc92d2ae915800a8ef8b6fdffe112c1b660df72297a3dcf2e2e3e1f959c6cb3678c63ee
905d672b74 test: use script_util helpers for creating P2W{PKH,SH} scripts (Sebastian Falbesoner)
285a65ccfd test: use script_util helpers for creating P2SH scripts (Sebastian Falbesoner)
b57b633b94 test: use script_util helpers for creating P2PKH scripts (Sebastian Falbesoner)
61b6a017a9 test: wallet util: fix multisig P2SH-P2WSH script creation (Sebastian Falbesoner)
Pull request description:
PR #18788 (commit 08067aebfd) introduced functions to generate output scripts for various types. This PR replaces all manual CScript creations in the P2PKH, P2SH, P2WPKH, P2WSH formats with those helpers in order to increase readability and maintainability over the functional test codebase. The first commit fixes a bug in the wallet_util helper module w.r.t. to P2SH-P2WSH script creation (the result is not used in any test so far, hence it can still be seen as refactoring).
The following table shows a summary of the output script patterns tackled in this PR:
| Type | master branch | PR branch |
| ---------- | ------------- | ------------- |
| P2PKH | `CScript([OP_DUP, OP_HASH160, hash160(key), OP_EQUALVERIFY, OP_CHECKSIG])` | `key_to_p2pkh_script(key)` |
| | `CScript([OP_DUP, OP_HASH160, keyhash, OP_EQUALVERIFY, OP_CHECKSIG])` | `keyhash_to_p2pkh_script(keyhash)` |
| P2SH | `CScript([OP_HASH160, hash160(script), OP_EQUAL])` | `script_to_p2sh_script(script)` |
| P2WPKH | `CScript([OP_0, hash160(key)])` | `key_to_p2wpkh_script(key)` |
| P2WSH | `CScript([OP_0, sha256(script)])` | `script_to_p2wsh_script(script)` |
Note that the `key_to_...` helpers can't be used if an invalid key size (not 33 or 65 bytes) is passed, which is the case in some rare instances where the scripts still have to be created manually.
Possible follow-up ideas:
* further simplify by identifying P2SH-wrapped scripts and using `key_to_p2sh_p2wpkh_script()` and `script_to_p2sh_p2wsh_script()` helpers
* introduce and use `key_to_p2pk_script()` helper for P2PK scripts
ACKs for top commit:
rajarshimaitra:
tACK 905d672b74
LarryRuane:
tACK 905d672b74
0xB10C:
ACK 905d672b74
MarcoFalke:
review ACK 905d672b74 🕹
Tree-SHA512: 7ccfe69699bc81168ac122b03536720013355c1b2fbb088355b616015318644c4d1cd27e20c4f56c89ad083ae609add4bc838cf6316794d0edb0ce9cf7fa0fd8
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
This reject reason is triggered for non-coinbase transactions with
a coinbase-like outpoint, i.e. hash=0, n=0xffffffff.
Note that the invalid tx templates are currently used in the
functional tests feature_block.py and p2p_invalid_tx.py.
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the
core implementation years ago due to being confusing and
superfluous, as it is implied by the block weight limit (see
PRs #10618 and #10608). Since there is also no point in
still keeping it in the functional test framework, we switch
to weight-based accounting on the relevant test code parts
and use `MAX_BLOCK_WEIGHT` instead for the block limit
checks.
c4ddee64c7 test: Add test for replacement relay fee check (Antoine Riard)
Pull request description:
This PR adds rename the `reject_reason` of our implementation of BIP125 rule 4 and adds missing functional test coverage. Note, `insufficient fee` is already the `reject_reason` of few others `PreChecks` replacement checks and as such might be confusing.
> The replacement transaction must also pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting. For example, if the minimum relay fee is 1 satoshi/byte and the replacement transaction is 500 bytes total, then the replacement must pay a fee at least 500 satoshis higher than the sum of the originals.
```
// Finally in addition to paying more fees than the conflicts the
// new transaction must pay for its own bandwidth.
CAmount nDeltaFees = nModifiedFees - nConflictingFees;
if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
hash.ToString(),
FormatMoney(nDeltaFees),
FormatMoney(::incrementalRelayFee.GetFee(nSize))));
}
```
ACKs for top commit:
MarcoFalke:
cr ACK c4ddee64c7
glozow:
ACK c4ddee6, one small suggestion if you retouch.
Tree-SHA512: 7c5d1065db6e6fe57a9f083bf051a7a55eb9892de3a2888679d4a6853491608c93b6e35887ef383a9988d14713fa13a0b1d6134b7354af5fd54765f0d4e98568
3efaf83c75 wallet: deactivate descriptor (S3RK)
6737d9655b test: wallet importdescriptors update existing (S3RK)
586f1d53d6 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db1474 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75
meshcollider:
Code review ACK 3efaf83c75
jonatack:
Light ACK 3efaf83c75 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
754f134a50 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d1072 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34 Add OutputType::BECH32M (Andrew Chow)
177c15d2f7 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50
jonatack:
ACK 754f134a50
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
fadddd13ee test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e test: Misc cleanup (MarcoFalke)
fa1668bf50 test: Run pep-8 (MarcoFalke)
facd97ae0f scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK fadddd13ee on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
bdb8b9a347 test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
1914054208 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
6168eb06b2 [test] Prevent intermittent issue (Amiti Uttarwar)
1d8193e2a2 [test] Remove GetAddrStore class (Amiti Uttarwar)
ef2f149bf2 [test] Update GetAddrStore callers to use AddrReceiver (Amiti Uttarwar)
e8c67ea19a [test] Add functionality to AddrReceiver (Amiti Uttarwar)
09dc073cff [test] Allow AddrReceiver to be used more generally (Amiti Uttarwar)
Pull request description:
A test refactor broken out from #21528 & a fix to #22243.
This PR:
1. consolidates the two helper classes into one, with the intent of making the test logic more clear & usable as we add more subtests to the file
2. hopefully fixes the test flakiness by bumping up the mocktime interval to ensure `m_next_addr_send` timer triggers
ACKs for top commit:
mzumsande:
Code-Review ACK 6168eb06b2
lsilva01:
Tested ACK 6168eb06b2 on Ubuntu 20.04
brunoerg:
tACK 6168eb06b2
Tree-SHA512: 248324f9d37e0e5ffe4acc437cd72ad9a2960abc868a97c6040a36e6ea8b59029127ac4f63fcf67d981a5bb4dbf2334bb2c23c541fae8e910d5523884bcedcba
d637a9b397 Taproot descriptor inference (Pieter Wuille)
c7388e5ada Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
bb719a08db style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10ebda3 doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957dcd test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977ae2 banman: save the banlist in a JSON format on disk (Vasil Dimov)
Pull request description:
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
ACKs for top commit:
jonatack:
Code review re-ACK bb719a08db per `git range-diff 6a67366 4b52c72 bb719a0`
achow101:
Code Review ACK bb719a08db
Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
fafd9165e9 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)
Pull request description:
Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)
ACKs for top commit:
jamesob:
crACK fafd9165e9
Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
The `on_addr` functionality of `AddrReceiver` tests logic specific to how the
addr messages are set up in the test bodies. To allow other callers to also use
`AddrReceiver`, only apply the assertion logic if the caller indicates
desirability by setting `test_addr_contents` to true when initializing the
class.
testshell in jupyter was failing due to an extra arg.
this adds a dummy -f param, which allows TestShell to
be used in a command line or jupyter environment
d6d2ab9845 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0e test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab9845
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
b9e76f1bf0 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7d rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef6 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59 rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf0
promag:
ACK b9e76f1bf0.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.
This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
fa7d71f270 test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649 test: Remove unused generate() from test (MarcoFalke)
faff3f35b7 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups 🎨
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270 thanks for the explanations, nicely done
theStack:
ACK fa7d71f270🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
55311197c4 Added new test for future blocks reacceptance (sanket1729)
511a5af462 Fixed inconsistencies between code and comments (sanket1729)
Pull request description:
This Commit does 3 things:
1) Adds a test case for checking reacceptance a previously rejected block which
was too far in the future.
~~2) clean up uses of rehash or calc_sha256 where it was not needed~~
3) While constructing block 44, this commit makes the code consistent with the expected figure in
the comment just above it by adding a transaction to the block.
4) Fix comment describing `sign_tx()` function
ACKs for top commit:
duncandean:
reACK 5531119
brunoerg:
reACK 55311197c4
Tree-SHA512: d40c72fcdbb0b2a0715adc58441eeea08147ee2ec5e371a4ccc824ebfdc6450698bd40aaeecb7ea7bfdb3cd1b264dd821b890276fff8b8d89b7225cdd9d6b546
451b96f7d2 test: kill process group to avoid dangling processes (S3RK)
Pull request description:
This is an alternative to #19281
This PR fixes a problem when after test failure with `--failfast` option there could be dangling nodes. The nodes will continue to occupy rpc/p2p ports on the machine and will cause further test failures.
If there are any dangling nodes left at the end of the test run we kill the whole process group.
Pros: the operations is immediate and won't lead to CI timeout
Cons: the test_runner process is also killed and exit code is 137
Example output:
```
...
Early exiting after test failure
TEST | STATUS | DURATION
rpc_decodescript.py | ✓ Passed | 2 s
rpc_deprecated.py | ✓ Passed | 2 s
rpc_deriveaddresses.py | ✓ Passed | 2 s
rpc_dumptxoutset.py | ✖ Failed | 2 s
ALL | ✖ Failed | 8 s (accumulated)
Runtime: 4 s
Killed: 9
> echo $?
137
```
ACKs for top commit:
MarcoFalke:
review ACK 451b96f7d2
aitorjs:
ACK 451b96f7d2. Manual testing with and without **--failfast**.
Tree-SHA512: 87e510a1411b9e7571e63cf7ffc8b9a8935daf9112ffc0f069d6c406ba87743ec439808181f7e13cb97bb200fad528589786c47f0b43cf3a2ef0d06a23cb86dd
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
754e802274 test: check rejected future block later accepted (Luke Dashjr)
Pull request description:
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
This PR re-opens https://github.com/bitcoin/bitcoin/pull/17872 which went stale and addresses the nits raised by reviewers there.
ACKs for top commit:
MarcoFalke:
review ACK 754e802274
Tree-SHA512: a2bbc8fffb523cf2831e1ecb05f20868e30106a38cc2e369e4973fa549cca06675a668df16f76c49cc4ce3a22925404255e5c53c4232d63ba1b9fca878509aa0
1) Makes the code for block 44 consistent with the expected figure in
the comment above it by adding a transaction to the block
2) Fixed comment describing sign_tx() function
