fa9b48549c test: Add test for -blockversion (MarcoFalke)
fa7fb0e442 test: Default blockversion to 4 in feature_block (MarcoFalke)
fa2b778d0c test: Remove unused -blockversion from tests (MarcoFalke)
Pull request description:
`-blockversion` is currently untested, as in: The setting could be made a no-op without any tests failing. Fix that by adding an explicit test for it. Also, related minor cleanups.
ACKs for top commit:
guggero:
ACK fa9b48549c.
Tree-SHA512: 1b2e792f7ed0ec1db163476ee8a938f8f7cb3691f797c721bbe55fdeed92487c2ff83b55467440096917999406c86430cb3a615383cefb4f621828309ff6a1e7
fa299ac273 test: Speed up wallet_resendwallettransactions test with mockscheduler RPC (MarcoFalke)
Pull request description:
Also fixes#20143
ACKs for top commit:
guggero:
ACK fa299ac2
Tree-SHA512: 024ced4aa5f5c266e24fd0583d47b45b19c2a6ae25a06fabeacaa0ac996eec0c45f11cc34b2df17d01759b78ed31a991aa86978aafcc76cb0017382f601bf85a
fa5a91a352 test: Fix typo (one tx is enough) in p2p_feefilter (MarcoFalke)
fa3af2c0d3 test: Fix intermittent issue in p2p_feefilter (MarcoFalke)
Pull request description:
Fixes:
```
Traceback (most recent call last):
File "test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "test/functional/p2p_feefilter.py", line 63, in run_test
self.test_feefilter()
File "test/functional/p2p_feefilter.py", line 117, in test_feefilter
txids = [miniwallet.send_self_transfer(fee_rate=Decimal('0.00020000'), from_node=node1)['wtxid'] for _ in range(3)]
File "test/functional/p2p_feefilter.py", line 117, in <listcomp>
txids = [miniwallet.send_self_transfer(fee_rate=Decimal('0.00020000'), from_node=node1)['wtxid'] for _ in range(3)]
File "test/functional/test_framework/wallet.py", line 63, in send_self_transfer
txid = from_node.sendrawtransaction(tx_hex)
File "test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: bad-txns-inputs-missingorspent (-25)
ACKs for top commit:
guggero:
ACK fa5a91a3
Tree-SHA512: 51d885753f72e1c91c4580709c15bdab60ff8c9d6f9bcb6db78a560e7e4dd7f76ce23add3303b374174afa3f11f74aa61db189a90c68d7f7655b15e64f51ed96
5aadd4be18 Convert amounts from float to decimal (Prayank)
Pull request description:
> decimal is preferred in accounting applications
https://docs.python.org/3.8/library/decimal.html
Decimal type saves an exact value so better than using float.
~~3 variables declared with type as 'Decimal' in [test/functional/mempool_accept.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_accept.py): fee, fee_expected, output_amount~~
~~Not required to convert to string anymore for using the above variables as decimal~~
+ fee, fee_expected, output_amount
~~+ 8 decimal places~~
+ Using value of coin['amount'] as decimal and removed 'int'
+ Removed unnecessary parentheses
+ Remove str() and use quotes
Fixes https://github.com/bitcoin/bitcoin/issues/20011
ACKs for top commit:
guggero:
ACK 5aadd4be18
Tree-SHA512: 5877cf3837e5b65bec0fc8909de141a720bfa02a747513e21d20f3c41ec0cfecc524d2c347a96596b0a1a97900da2acf08b799f26b11d537e4dcddc6ce45f38e
+ fee, fee_expected, output_amount
+ Using value of coin['amount'] as decimal and removed 'int'
+ Removed unnecessary parentheses
+ Remove str() and use quotes
Testing that requests to very old blocks / block headers fail can simply be
done by checking that the node doesn't respond with any "blocks" / "headers"
message at all.
Also removes unnecessary sending of block/header requests and replaces
time.sleep(3) with node0.sync_with_ping().
fa4074b395 Show name, format and if uses descriptors in bitcoin-wallet tool (Jonas Schnelli)
Pull request description:
ACKs for top commit:
MarcoFalke:
ACK fa4074b395
jonatack:
re-ACK fa4074b395
Tree-SHA512: cf6ee96ff21532fc4b0ba7a0fdfdc1fa485c9b1495447350fe65cd0bd919e0e0280613933265cdee069b8c29ccf015ac374535a70cac3d4fb89f4d08b3a03519
-BEGIN VERIFY SCRIPT-
# max-depth=0 excludes test/functional/test_framework/...
FILES=$(git grep -l --max-depth 0 "connect_nodes" test/functional)
# Replace (dis)?connect_nodes(self.nodes[a], b) with self.(dis)?connect_nodes(a, b)
sed -i 's/\b\(dis\)\?connect_nodes(self\.nodes\[\(.*\)\]/self.\1connect_nodes(\2/g' $FILES
# Remove imports in the middle of a line
sed -i 's/\(dis\)\?connect_nodes, //g' $FILES
sed -i 's/, \(dis\)\?connect_nodes//g' $FILES
# Remove imports on a line by themselves
sed -i '/^\s*\(dis\)\?connect_nodes,\?$/d' $FILES
sed -i '/^from test_framework\.util import connect_nodes$/d' $FILES
-END VERIFY SCRIPT-
Co-authored-by: Elliott Jin <elliott.jin@gmail.com>
A later scripted-diff commit replaces the majority of uses, which all
follow this pattern:
(dis)?connect_nodes(self.nodes[a], b)
This commit replaces the few "special cases".
624bab00dd test: add coverage for getwalletinfo format field (Jon Atack)
5e737a0092 rpc, wallet: Expose database format in getwalletinfo (João Barbosa)
Pull request description:
Support for sqlite based wallets was added in #19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`.
ACKs for top commit:
jonatack:
Tested ACK 624bab00dd
laanwj:
Code review ACK 624bab00dd.
MarcoFalke:
doesn't hurt ACK 624bab00dd
hebasto:
ACK 624bab00dd, tested on Linux Mint 20 (x86_64).
meshcollider:
utACK 624bab00dd
Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
fa48405ef8 Warn on unknown rw_settings (MarcoFalke)
Pull request description:
Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.
Something similar is already done for the command line and config file. See:
* test: Add test for unknown args #16234 (commit fa7dd88b71)
ACKs for top commit:
ryanofsky:
Code review ACK fa48405ef8. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
Tree-SHA512: cec7d88adf84fa0a842f56b26245157736eb50df433db951e622ea07fd145b899822b24cdab1d8b36c066415ce4f0ef09b493fa8a8d691532822a59c573aafa7
b128b56672 test: add logging for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
8ee3536b2b test: remove unused helpers random_transaction(), make_change() and gather_inputs() (Sebastian Falbesoner)
fddce7e199 test: use MiniWallet for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_getblocktemplate_longpoll.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078. Also adds missing log messages for the subtests.
This was the only functional test that used the `random_transaction` helper in `test_framework/util.py`, hence it is removed, together with other helpers (`make_change` and `gather_inputs`) that were again only used by `random_transaction`.
ACKs for top commit:
MarcoFalke:
ACK b128b56672
Tree-SHA512: 09a5fa7b0f5976a47040f7027236d7ec0426d5a4829a082221c4b5fae294470230e89ae3df0bca0eea26833162c03980517f5cc88761ad251c3df4c4a49bca46
d438d609cd QA: Use GBT to get block versions correct (Luke Dashjr)
1df2cd1c8f QA: blocktools: Accept block template to create_block (Luke Dashjr)
Pull request description:
The goal here is to decouple unrelated tests from the details of block versions.
Currently, these tests are forcing specific versions of blocks for no real reason.
ACKs for top commit:
fjahr:
re-ACK d438d609cd
benthecarman:
ACK d438d60
Tree-SHA512: 523b1cd4dac8d65c88432e126ce7f60df96ca4b94f7ecc8e83ba4ffbade23e2afe7055fdf586ce3c195a533f2004e63fff83add4267b39473a581c9f1c6d5340
6272604bef refactor: enable -netinfo to add future networks (i2p, cjdns) (Jon Atack)
82fd40216c refactor: promote some -netinfo localvars to class members (Jon Atack)
5133fab37e cli: simplify -netinfo using getpeerinfo network field (Jon Atack)
4938a109ad rpc, test: expose CNodeStats network in RPC getpeerinfo (Jon Atack)
6df7882029 net: add peer network to CNodeStats (Jon Atack)
Pull request description:
This PR:
- builds on #19991 and #19998
- exposes peer networks via a new getpeerinfo `network` field ("ipv4", "ipv6", or "onion"), and adds functional tests
- updates -netinfo to use getpeerinfo `network` rather than detecting the peer networks client-side
- refactors -netinfo to easily add future networks
ACKs for top commit:
laanwj:
ACK 6272604bef
Tree-SHA512: 28883487585135ceaaf84ce09131f2336e3193407f2e3df0960e3f4ac340f500ab94ffecb9d06a4c49bc05e3cca4f914ea4379860bea0bd5df2f834f74616015
5b57dc5458 RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219 RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
0e2a5e448f tests: dumping and minimizing of script assets data (Pieter Wuille)
4567ba034c tests: add generic qa-asset-based script verification unit test (Pieter Wuille)
f06e6d0345 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille)
3c226639eb tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille)
206fb180ec --- [TAPROOT] Tests --- (Pieter Wuille)
d7ff237f29 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille)
e9a021d7e6 Make Taproot spends standard + policy limits (Pieter Wuille)
865d2c37e2 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille)
72422ce396 Implement Tapscript script validation rules (BIP 342) (Johnson Lau)
330de894a9 Use ScriptExecutionData to pass through annex hash (Pieter Wuille)
8bbed4b7ac Implement Taproot validation (BIP 341) (Pieter Wuille)
0664f5fe1f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille)
5de246ca81 Implement Taproot signature hashing (BIP 341) (Johnson Lau)
9eb590894f Add TaggedHash function (BIP 340) (Pieter Wuille)
450d2b2371 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille)
5d62e3a68b refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille)
8bd2b4e784 refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille)
107b57df9f scripted-diff: put ECDSA in name of signature functions (Pieter Wuille)
f8c099e220 --- [TAPROOT] Refactors --- (Pieter Wuille)
Pull request description:
This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
See the list of commits [below](https://github.com/bitcoin/bitcoin/pull/19953#issuecomment-691815830). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.
This is a successor to https://github.com/bitcoin/bitcoin/pull/17977 (see discussion following [this comment](https://github.com/bitcoin/bitcoin/pull/17977#issuecomment-682285983)), and will have further changes squashed/rebased. The history of this PR can be found in #19997.
ACKs for top commit:
instagibbs:
reACK 0e2a5e448f
benthecarman:
reACK 0e2a5e4
kallewoof:
reACK 0e2a5e448f
jonasnick:
ACK 0e2a5e448f almost only looked at bip340/libsecp related code
jonatack:
ACK 0e2a5e448f modulo the last four commits (tests) that I plan to finish reviewing tomorrow
fjahr:
reACK 0e2a5e448f
achow101:
ACK 0e2a5e448f
Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e
c4a29d0a90 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky)
310b0fde04 Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow)
6c6639ac9f Include sqlite3 in documentation (Andrew Chow)
f023b7cac0 wallet: Enforce sqlite serialized threading mode (Andrew Chow)
6173269866 Set and check the sqlite user version (Andrew Chow)
9d3d2d263c Use network magic as sqlite wallet application ID (Andrew Chow)
9af5de3798 Use SQLite for descriptor wallets (Andrew Chow)
9b78f3ce8e walletutil: Wallets can also be sqlite (Andrew Chow)
ac38a87225 Determine wallet file type based on file magic (Andrew Chow)
6045f77003 Implement SQLiteDatabase::MakeBatch (Andrew Chow)
727e6b2a4e Implement SQLiteDatabase::Verify (Andrew Chow)
b4df8fdb19 Implement SQLiteDatabase::Rewrite (Andrew Chow)
010e365906 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow)
ac5c1617e7 Implement SQLiteDatabase::Backup (Andrew Chow)
f6f9cd6a64 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow)
bf90e033f4 Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow)
7aa45620e2 Add SetupSQLStatements (Andrew Chow)
6636a2608a Implement SQLiteBatch::Close (Andrew Chow)
93825352a3 Implement SQLiteDatabase::Close (Andrew Chow)
a0de83372b Implement SQLiteDatabase::Open (Andrew Chow)
3bfa0fe125 Initialize and Shutdown sqlite3 globals (Andrew Chow)
5a488b3d77 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow)
ca8b7e04ab Implement SQLiteDatabaseVersion (Andrew Chow)
7577b6e1c8 Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow)
e87df82580 Add sqlite to travis and depends (Andrew Chow)
54729f3f4e Add libsqlite3 (Andrew Chow)
Pull request description:
This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`.
For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite.
We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use.
I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier.
ACKs for top commit:
Sjors:
re-utACK c4a29d0a90
promag:
Tested ACK c4a29d0a90.
fjahr:
reACK c4a29d0a90
S3RK:
Re-review ACK c4a29d0a90
meshcollider:
re-utACK c4a29d0a90
hebasto:
re-ACK c4a29d0a90, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19077#pullrequestreview-507743699) review, verified with `git range-diff master d18892dcc c4a29d0a9`.
ryanofsky:
Code review ACK c4a29d0a90. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction.
jonatack:
ACK c4a29d0a90, debug builds and test runs after rebase to latest master @ c2c4dbaebd, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns.
Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
fd9a0060f0 Report and verify expirations (Pieter Wuille)
86f50ed10f Delete limitedmap as it is unused now (Pieter Wuille)
cc16fff3e4 Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille)
173a1d2d3f Expedite removal of tx requests that are no longer needed (Pieter Wuille)
de11b0a4ef Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille)
242d16477d Change transaction request logic to use txrequest (Pieter Wuille)
5b03121d60 Add txrequest fuzz tests (Pieter Wuille)
3c7fe0e5a0 Add txrequest unit tests (Pieter Wuille)
da3b8fde03 Add txrequest module (Pieter Wuille)
Pull request description:
This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests).
The major changes are:
* Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first.
* No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available).
* The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions).
This replaces #19184, rebased on #18044 and with many small changes.
ACKs for top commit:
ariard:
Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic.
MarcoFalke:
Approach ACK fd9a0060f0🏹
naumenkogs:
Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events.
jnewbery:
utACK fd9a0060f0
jonatack:
WIP light ACK fd9a0060f0 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers.
ryanofsky:
Light code review ACK fd9a0060f0, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on:
Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7
5b77f8098d test: add p2p_lock acquires in p2p_leak_tx.py (Sebastian Falbesoner)
cc8c6823b4 test: use MiniWallet for p2p_leak_tx.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_leak_tx.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078. It also adds missing p2p_lock acquires that need to be held while modifying internal p2p Interface state (in this case the `last_message` dictionary) to avoid data races.
ACKs for top commit:
laanwj:
Code review ACK 5b77f8098d
Tree-SHA512: 6661bc6e3491a9af4bf040f379e5955c525136397e99d3eadde92e247580d0d87efff750e6d3b1f6d9a4e578144a433a982f574ef056b44dd6bca33873a1bae6
This adds a --dumptests flag to the feature_taproot.py test, to dump all its
generated test cases to files, in a format compatible with the
script_assets_test unit test. A fuzzer for said format is added as well, whose
primary purpose is coverage-based minimization of those dumps.
A large functional test is added that automatically generates random transactions which
exercise various aspects of the new rules, and verifies they are accepted into the mempool
(when appropriate), and correctly accepted/rejected in (Python-constructed) blocks.
Includes sighashing code and many tests by Johnson Lau.
Includes a test by Matthew Zipkin.
Includes several tests and improvements by Greg Sanders.
This adds a `TxoutType::WITNESS_V1_TAPROOT` for P2TR outputs, and permits spending
them in standardness rules. No corresponding `CTxDestination` is added for it,
as that isn't needed until we want wallet integration. The taproot validation flags
are also enabled for mempool transactions, and standardness rules are added
(stack item size limit, no annexes).
Maintaining up to 100000 INVs per peer is excessive, as that is far more
than fits in a typical mempool.
Also disable the "overload" penalty for PF_RELAY peers.
This removes most transaction request logic from net_processing, and
replaces it with calls to a global TxRequestTracker object.
The major changes are:
* Announcements from outbound (and whitelisted) peers are now always
preferred over those from inbound peers. This used to be the case for the
first request (by delaying the first request from inbound peers), and
a bias afters. The 2s delay for requests from inbound peers still exists,
but after that, if viable outbound peers remain for any given transaction,
they will always be tried first.
* No more hard cap of 100 in flight transactions per peer, as there is less
need for it (memory usage is linear in the number of announcements, but
independent from the number in flight, and CPU usage isn't affected by it).
Furthermore, if only one peer announces a transaction, and it has over 100
in flight and requestable already, we still want to request it from them.
The cap is replaced with an additional 2s delay (possibly combined with the
existing 2s delays for inbound connections, and for txid peers when wtxid
peers are available).
Includes functional tests written by Marco Falke and Antoine Riard.
dcf0cb4776 tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
353a3fdaad net: advertise support for ADDRv2 via new message (Vasil Dimov)
201a4596d9 net: CAddress & CAddrMan: (un)serialize as ADDRv2 (Vasil Dimov)
1d3ec2a1fd Support bypassing range check in ReadCompactSize (Pieter Wuille)
Pull request description:
This PR contains the two remaining commits from #19031 to complete the [BIP155](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) implementation:
`net: CAddress & CAddrMan: (un)serialize as ADDRv2`
`net: advertise support for ADDRv2 via new message`
plus one more commit:
`tor: make a TORv3 hidden service instead of TORv2`
ACKs for top commit:
jonatack:
re-ACK dcf0cb4776 per `git diff 9b56a68 dcf0cb4` only change since last review is an update to the release notes which partially picked up the suggested text. Running a node on this branch and addnode-ing to 6 other Tor v3 nodes, I see "addrv2" and "sendaddrv2" messages in getpeerinfo in both the "bytesrecv_per_msg" and "bytessent_per_msg" JSON objects.
sipa:
ACK dcf0cb4776
hebasto:
re-ACK dcf0cb4776, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed.
laanwj:
Edit: I have to retract this ACK for now, I'm having some problems with this PR on a FreeBSD node. It drops all outgoing connections with this dcf0cb4776 merged on master (12a1c3ad1a).
ariard:
Code Review ACK dcf0cb4
Tree-SHA512: 28d4d0d817b8664d2f4b18c0e0f31579b2f0f2d23310ed213f1f436a4242afea14dfbf99e07e15889bc5c5c71ad50056797e9307ff8a90e96704f588a6171308
Introduce a new message `sendaddrv2` to signal support for ADDRv2.
Send the new message immediately after sending the `VERACK` message.
Add support for receiving and parsing ADDRv2 messages.
Send ADDRv2 messages (instead of ADDR) to a peer if he has
advertised support for it.
Co-authored-by: Carl Dong <contact@carldong.me>
907f142fc7 rpc: change no wallet loaded message to be clearer (Andrew Chow)
Pull request description:
Changes the no wallet is loaded rpc error message to be clearer that no wallet is loaded and how the user can load or create a wallet. Also changes the error code from METHOD_NOT_FOUND to RPC_WALLET_NOT_FOUND as that makes more sense.
ACKs for top commit:
MarcoFalke:
review ACK 907f142fc7
kristapsk:
ACK 907f142fc7. In addition to standard tests, just in case tested that this doesn't break anything with JoinMarket.
meshcollider:
utACK 907f142fc7
Tree-SHA512: 4b413e6ab5430ec75a79de9db6583f2f3f38ccdf71aa373d8386a56e64f07f92200c8107c8c82c92c7c431d739615977c208b771a24c5960fa8676789b5497a2
Changes the no wallet is loaded rpc error message to be clearer that no
wallet is loaded and how the user can load or create a wallet. Also
changes the error code from METHOD_NOT_FOUND to RPC_WALLET_NOT_FOUND as
that makes more sense.
b048b275d9 [validation] Remove absurdfee from accepttomempool (John Newbery)
932564b9cf scripted-diff: update max-fee-exceeded error message to include RPC (gzhao408)
8f1290c601 [rpc/node] check for high fee before ATMP in clients (gzhao408)
Pull request description:
Picked up from #15810. Add separate fee-checking logic for clients that need to enforce max fee rates, then remove the `absurdFee` logic from ATMP.
ATMP's `nAbsurdFee` argument is used to enforce user-specific behavior (it is not policy since it isn't applied consistently: it is only ever used in RPC and wallet, and set to 0 everywhere else internally). It should be removed from `AcceptToMemoryPool` because (1) validation results/mempool behavior should not be user-specific and (2) enforcing a max fee rate should be the responsibility of the client instead of the mempool.
Note: this PR does not intend to _remove_ protection from high fees, just re-delegate the responsibility to clients.
ACKs for top commit:
jnewbery:
utACK b048b275d9
LarryRuane:
re-ACK b048b275d9
MarcoFalke:
re-ACK b048b275d9 , only change is squashing one commit 🏦
instagibbs:
utACK b048b275d9
Tree-SHA512: 57c17ba16d230a4cae2896dd6a64c924f307757824e35784bf96da7b10aff2d8ea910710edf35e981035623a155f8766209a92a0fdb856549fde78bc3eaae4d2
-BEGIN VERIFY SCRIPT-
sed -i 's/Fee exceeds maximum configured by \-\maxtxfee/Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)/g' src/util/error.cpp test/functional/rpc_fundrawtransaction.py test/functional/rpc_psbt.py test/functional/rpc_rawtransaction.py test/functional/wallet_bumpfee.py test/functional/wallet_create_tx.py
-END VERIFY SCRIPT-
Check absurd fee in BroadcastTransaction and RPC,
return TransactionError::MAX_FEE_EXCEEDED instead
of TxValidationResult::TX_NOT_STANDARD because this
is client preference, not a node-wide policy.
a56e9f5670 test: Assert exclusive PSBT funding options (Oliver Gugger)
64bc5efd39 test: Assert PSBT change type (Oliver Gugger)
Pull request description:
Increases test coverage of the `walletcreatefundedpsbt` RPC.
Tests the following combinations:
- Make sure the global option `-changetype` is used as the default value for the `change_type` option if not specified.
- Make sure the global option `-changetype` can be overwritten by explicitly setting the `change_type` option of the `walletcreatefundedpsbt` RPC call.
- Make sure the options `change_type` and `changeAddress` are mutually exclusive.
ACKs for top commit:
achow101:
ACK a56e9f5670
Tree-SHA512: bf0fb20c890887b7228ad9277fdb32f367ba772eed6efbe2b4f471f808d4d435110256601e8ebd9bea57026d9f22f3cc3c26a009b017e3da6d8fc6896313def5
f471a3be00 scripted diff: Improve invalid vout value rpc error message (Nima Yazdanmehr)
Pull request description:
Since the `vout` value can start at `0`, the error message for *negative* values can be improved to something like: `vout cannot be negative`.
ACKs for top commit:
fanquake:
ACK f471a3be00
promag:
Code review ACK f471a3be00.
Tree-SHA512: fbdee3d0ddd5b58eb93934a1217b44e125a9ad39e672b1f35c7609c6c5fcf45ae1b731d3d6135b7225d98792dbfc34a50907b8c41274a5b029d7b5c59f886560
c1585bca8d test: Get rid of default wallet hacks (Russell Yanofsky)
ed3acda33b test, refactor: add default_wallet_name and wallet_data_filename variables (Russell Yanofsky)
Pull request description:
Changes:
- Get rid of setup_nodes (`-wallet`, `-nowallet`, `-disablewallet`) argument rewriting
- Get rid of hardcoded wallet `""` names and `-wallet=""` args
Motivation:
- Simplify test framework behavior so it's easier to write new tests without having arguments mangled by the framework
- Make tests more readable, replacing unexplained `""` string literals with `default_wallet_name` references
- Make it trivial to update default wallet name and wallet data filename for sqlite #19077 testing
- Stop relying on `-wallet` arguments to create wallets, so it is easy to change `-wallet` option in the future to only load existing wallets not create new ones (to avoid accidental wallet creation, and encourage use of wallet encryption and descriptor features)
ACKs for top commit:
MarcoFalke:
ACK c1585bca8d, only effective change is adding documentation 🎵
Tree-SHA512: f62dec7cbdacb5f330aa0e1eec89ab4d065540d91495bbedcb375eda1c080b45ce9edb310ce253c44c4839f1b4cc2c7df9816c58402d5d43f94a437e301ea8bc
e66870c5a4 zmq: Append address to notify log output (nthumann)
241803da21 test: Add zmq test to support multiple interfaces (nthumann)
a0b2e5cb6a doc: Add release notes to support multiple interfaces (nthumann)
b1c3f180ec doc: Adjust ZMQ usage to support multiple interfaces (nthumann)
347c94f551 zmq: Add support to listen on multiple interfaces (Nicolas Thumann)
Pull request description:
This PR adds support for ZeroMQ to listen on multiple interfaces, just like the RPC server.
Currently, if you specify more than one e.g. `zmqpubhashblock` paramter, only the first one will be used. Therefore a user may be forced to listen on all interfaces (e.g. `zmqpubhashblock=0.0.0.0:28332`), which can result in an increased attack surface.
With this PR a user can specify multiple interfaces to listen on, e.g.
`-zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubhashblock=tcp://192.168.1.123:28332`.
ACKs for top commit:
laanwj:
Code review ACK e66870c5a4
instagibbs:
reACK e66870c5a4
Tree-SHA512: f38ab4a6ff00dc821e5f4842508cefadb701e70bb3893992c1b32049be20247c8aa9476a1f886050c5f17fe7f2ce99ee30193ce2c81a7482a5a51f8fc22300c7
6fccad7f71 signet: do not log signet startup messages for other chains (Jon Atack)
Pull request description:
The following signet startup messages are printed to the debug log immediately on node startup for all chains. This behavior occurs on master as a side effect after the merge of #20014. This PR removes the first message and moves the signet derived magic logging to `init.cpp`.
```
$ ./src/bitcoind
2020-09-30T14:25:15Z Using default signet network
2020-09-30T14:25:15Z Signet derived magic (message start): 0a03cf40
2020-09-30T14:25:15Z Bitcoin Core version v0.20.99.0 ...
```
ACKs for top commit:
MarcoFalke:
ACK 6fccad7f71
kallewoof:
utACK 6fccad7f71
hebasto:
ACK 6fccad7f71
Tree-SHA512: 33821dce89b24caf7b7c1ecb41e572ecfb26e6958a1316d359ff240e6ef97c4a1f2cf1b4b974596b252815f9df23960ce385c132ebdbc855bbe6123c3b0b003a
-BEGIN VERIFY SCRIPT-
r() { sed -i 's/vout must be positive/vout cannot be negative/g' $1 }
r $(git grep -l 'vout must be positive')
-END VERIFY SCRIPT-
69cf5d4eeb [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
9b4fa0af40 net: Print error message if -proxy is specified without arguments (instead of continuing without proxy server) (practicalswift)
Pull request description:
Exit with error message if `-proxy` is specified without arguments (instead of continuing without proxy server).
Continuing without a proxy server when the end-user has specified `-proxy` may result in accidental loss of privacy. (The end-user might think he/she is using a proxy when he/she is not.)
Before this patch:
```
$ src/bitcoind -proxy
…
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -listen=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -upnp=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
2020-09-23T00:24:33Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
…
2020-09-23T00:24:33Z init message: Starting network threads...
```
`bitcoind` is now running *without* a proxy server (`GetProxy(…, …) == false`, `HaveNameProxy() == false`, etc.).
Note that the "-proxy set" log messages above which the end-user might interpret as "good, my traffic is now routed via the proxy".
After this patch:
```
$ src/bitcoind -proxy
Error: No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>.
$ echo $?
1
```
ACKs for top commit:
laanwj:
re-ACK 9b4fa0af40
kristapsk:
ACK 9b4fa0af40, I have tested the code.
hebasto:
re-ACK 9b4fa0af40
Tree-SHA512: 4ba7a011991699a54b5bb87ec68367c681231bf5dcd36f8c89ff9ddc2e8d29df453817b7e362597e652ad6b341a22b7274be0fd78d435e5f0fd8058e5221c4ce
- Get rid of hardcoded wallet "" names and -wallet="" args
- Get rid of setup_nodes (-wallet, -nowallet, -disablewallet) argument rewriting
Motivation:
- Simplify test framework behavior so it's easier to write new tests without
having arguments mangled by the framework
- Make tests more readable, replacing unexplained "" string literals with
default_wallet_name references
- Make it trivial to update default wallet name and wallet data filename for
sqlite #19077 testing
- Stop relying on -wallet arguments to create wallets, so it is easy to change
-wallet option in the future to only load existing wallets not create new
ones (to avoid accidental wallet creation, and encourage use of wallet
encryption and descriptor features)
deb52711a1 Remove header checks out of net_processing (Troy Giorshev)
52d4ae46ab Give V1TransportDeserializer CChainParams& member (Troy Giorshev)
5bceef6b12 Change CMessageHeader Constructor (Troy Giorshev)
1ca20c1af8 Add doxygen comment for ReceiveMsgBytes (Troy Giorshev)
890b1d7c2b Move checksum check from net_processing to net (Troy Giorshev)
2716647ebf Give V1TransportDeserializer an m_node_id member (Troy Giorshev)
Pull request description:
Inspired by #15206 and #15197, this PR moves all message header verification from the message processing layer and into the network/transport layer.
In the previous PRs there is a change in behavior, where we would disconnect from peers upon a single failed checksum check. In various discussions there was concern over whether this was the right choice, and some expressed a desire to see how this would look if it was made to be a pure refactor.
For more context, see https://bitcoincore.reviews/15206.html#l-81.
This PR improves the separation between the p2p layers, helping improvements like [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and #18989.
ACKs for top commit:
ryanofsky:
Code review ACK deb52711a1 just rebase due to conflict on adjacent line
jnewbery:
Code review ACK deb52711a1.
Tree-SHA512: 1a3b7ae883b020cfee1bef968813e04df651ffdad9dd961a826bd80654f2c98676ce7f4721038a1b78d8790e4cebe8060419e3d8affc97ce2b9b4e4b72e6fa9f
f7b331ea85 rpc: add brackets to ConstructTransaction (Sjors Provoost)
d813d26f06 [rpc] send: various touch-ups (Sjors Provoost)
0fc1c685e1 [rpc] send: fix parsing replaceable option (Sjors Provoost)
efc9b85e6f Mark send RPC experimental (Sjors Provoost)
Pull request description:
Followup based on #16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough).
I marked the RPC as experimental so we can tweak it a bit over the next release cycle.
ACKs for top commit:
meshcollider:
utACK f7b331ea85
fjahr:
utACK f7b331ea85
kallewoof:
ACK f7b331ea85
Tree-SHA512: 82dd8ac76a6558872db3f5249d4d6440469400aaa339153bc627d1ee673a91ecfadecb486bc1939ba87ebbd80e26ff29698e93e358599f3d26fde0e526892afe
92e28fa8b2 test: remove unused constants in functional tests (Sebastian Falbesoner)
Pull request description:
This mini-PR gets rid of constants in functional tests that are not used anymore. Found by [vulture ](https://pypi.org/project/vulture/)via the following script that has been lying around here locally for quite some time (I think it was once proposed by practicalswift, but I don't remember the concrete topic/PR):
```
#!/bin/sh
for F in $(git ls-files -- "*.py"); do vulture "$F" | grep "unused variable"; done
```
ACKs for top commit:
practicalswift:
ACK 92e28fa8b2: patch looks correct.
Tree-SHA512: 16516abc8014207bcefdf0545dffaecff1fbba66f45b54c02371dcfd1f18194855c6b72598c11b5407009561eafe8048d47af3471f0efb1795d52477d5a0232e
a512925e19 [doc] Release notes (Amiti Uttarwar)
50f94b34a3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b50 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca4 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19.
sipa:
utACK a512925e19
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19🌇
promag:
Code review ACK a512925e19.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
10d61505fe [test] remove confusing p2p property (gzhao408)
549d30faf0 scripted-diff: replace p2p with p2ps[0] in p2p_invalid_tx (gzhao408)
7a0de46aea [doc] sample code for test framework p2p objects (gzhao408)
784f757994 [refactor] clarify tests by referencing p2p objects directly (gzhao408)
Pull request description:
The `TestNode` has a `p2p` property which is an alias for `p2ps[0]`.
I think this should be removed because it can be confusing and misleading (to both the test writer and reviewer), especially if a TestNode has multiple p2ps connected (which is the case for many tests).
Another example is when a test has multiple subtests that connect 1 p2p and use the `p2p` property to reference it. If the subtests don't completely clean up after themselves, the subtests may affect one another.
The best way to refer to a connected p2p is use the object returned by `add_p2p_connection` like this:
```py
p2p_conn = node.add_p2p_connection(P2PInterface())
```
A good example is [p2p_invalid_locator.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_locator.py), which cleans up after itself (waits in both `wait_for_disconnect` and in `disconnect_p2ps`) but wouldn't need so much complexity if it just referenced the connections directly.
If there is only one connected, it's not really that tedious to just use `node.p2ps[0]` instead of `node.p2p` (and it can always be aliased inside the test itself).
ACKs for top commit:
robot-dreams:
utACK 10d61505fe
jnewbery:
utACK 10d61505fe
guggero:
Concept ACK 10d61505.
Tree-SHA512: 5965548929794ec660dae03467640cb2156d7d826cefd26d3a126472cbc2494b855c1d26bbb7b412281fbdc92b9798b9765a85c27bc1a97f7798f27f64db6f13
759d94e70f Update zmq notification documentation and sample consumer (Gregory Sanders)
68c3c7e1bd Add functional tests for zmq sequence topic and mempool sequence logic (Gregory Sanders)
e76fc2b84d Add 'sequence' zmq publisher to track all block (dis)connects, mempool deltas (Gregory Sanders)
1b615e61bf zmq test: Actually make reorg occur (Gregory Sanders)
Pull request description:
This PR creates a new ZMQ notifier that gives a "total hash history" of block (dis)connection, mempool addition/substraction, all in one pipeline. It also exposes a "mempool sequence number" to both this notifier and `getrawmempool` results, which allows the consumer to use the results together without confusion about ordering of results and without excessive `getrawmempool` polling.
See the functional test `interfaces_zmq.py::test_mempool_sync` which shows the proposed user flow for the client-side tracking of mempool contents and confirmations.
Inspired by https://github.com/bitcoin/bitcoin/pull/19462#issuecomment-656140421
Alternative to https://github.com/bitcoin/bitcoin/pull/19462 due to noted deficiencies in current zmq notification streams.
Also fixes a legacy zmq test that didn't actually trigger a reorg because of identical blocks being generated on each side of the split(oops)
ACKs for top commit:
laanwj:
Code review ACK 759d94e70f
Tree-SHA512: 9daf0d7d996190f3a68ff40340a687519323d7a6c51dcb26be457fbc013217ea7b62fbd0700b74b654433d2e370704feb61e5584399290692464fcfcb72ce3b7
facaf9e61f doc: Document signet BIP (MarcoFalke)
faf0a26711 doc: Update comments for new chain settings (-signet and -chain=signet) (MarcoFalke)
fae0548686 fuzz: Remove needless guard (MarcoFalke)
77771a03df refactor: Remove SignetTxs::m_valid and use optional instead (MarcoFalke)
fa2ad5dae1 test: Run signet test even when wallet was not compiled (MarcoFalke)
Pull request description:
Some doc and test fixups for #18267
ACKs for top commit:
ajtowns:
ACK facaf9e61f -- code review only
dr-orlovsky:
Reviewed & ACK facaf9e61f
kallewoof:
Code review ACK facaf9e61f
Tree-SHA512: 8085027c488d84bb4bddccba78bd2d4c5af0d8e2644ee72265f1f30fa8c83f61a961d9da2c796f2940e69682291cbee7b1028b6a6ce123ad9134c0ebbf4723b0
This moves header size and netmagic checking out of net_processing and
into net. This check now runs in ReadHeader, so that net can exit early
out of receiving bytes from the peer. IsValid is now slimmed down, so
it no longer needs a MessageStartChars& parameter.
Additionally this removes the rest of the m_valid_* members from
CNetMessage.
e15344889a Clarify blocksonly whitelistforcerelay test (t-bast)
Pull request description:
As discussed in https://github.com/bitcoin/bitcoin/issues/19943, this test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of Bitcoin Core to actually relay transactions to a `blocksonly` node and benefit from the `whitelistforcerelay` parameter.
ACKs for top commit:
naumenkogs:
ACK e15344889a
Tree-SHA512: cc3526ac26c40a2d878b0ad863008663040683fd21092fcdc93866c2b0a79db8c2d29767d1f70bf56195092fca2aa2961cdbee52b5f0b1ae757cece9cd206301
0d04784af1 Refactor the functional test (Gleb Naumenko)
83ad65f31b Address nits in ADDR caching (Gleb Naumenko)
81b00f8780 Add indexing ADDR cache by local socket addr (Gleb Naumenko)
42ec558542 Justify the choice of ADDR cache lifetime (Gleb Naumenko)
Pull request description:
This is a follow-up on #18991 which does 3 things:
- improves privacy of a node listening to multiple addresses via adding cache index by local socket addr (suggested [here](https://github.com/bitcoin/bitcoin/pull/18991#issuecomment-668219345))
- documents on the choice of 24h cache lifetime
- addresses nits from #18991
ACKs for top commit:
jnewbery:
utACK 0d04784af1
vasild:
ACK 0d04784
jonatack:
Code review ACK 0d04784
Tree-SHA512: bb65a34dd1ce2811186d3e4469bc33e8399cebaaa494ce13041c7cff23275870e4176a719f7a72f8d779c49f8b2344bf4fa1aeb3ea4e2626d5ae76514f00a750
As discussed in https://github.com/bitcoin/bitcoin/issues/19943, this
test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of
Bitcoin Core to actually relay transactions to a `blocksonly` node and
benefit from the `whitelistforcerelay` parameter.
638441928a test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner)
Pull request description:
While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.
ACKs for top commit:
guggero:
Code review ACK 638441928a.
epson121:
Code review ACK 638441928a
Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
23c35bf005 [test] add get_vsize util for more programmatic testing (gzhao408)
2233a93a10 [rpc] Return fee and vsize from testmempoolaccept (codeShark149)
Pull request description:
From #19093 and resolves#19057.
Difference from #19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md.
ACKs for top commit:
jnewbery:
utACK 23c35bf005
fjahr:
utACK 23c35bf
instagibbs:
reACK 23c35bf005
Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04
d26f0648f1 Tell users how to load or create a wallet when no wallet is loaded (Andrew Chow)
1bee1e6269 Do not create default wallet (Andrew Chow)
Pull request description:
Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.
Builds on #19754 which provides the `load_on_startup` behavior for the GUI.
ACKs for top commit:
jnewbery:
Manual test and very light code review ACK d26f0648f1
ryanofsky:
Code review ACK d26f0648f1. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit
jonatack:
ACK d26f0648f1 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian.
Tree-SHA512: 091d785aef64736f7df661c576e815a87f3d029cfa32f3a75ba86fc25795f10b022ab3ae15c5b61a10b8cee16f5650f15cd79cbd6127e5e3ccbef631966d3c30
e1fdd2963b Test batch rpc with params (Gregory Sanders)
Pull request description:
Useful as an example and test case.
ACKs for top commit:
laanwj:
ACK e1fdd2963b
theStack:
ACK e1fdd2963b
Tree-SHA512: 2d2ba8960916342b264a14624857d6dd10005be12efafb3e970b82656f721c8f3700ebc9b8809de1b2f887d482b772043504aeaeebc7f2e1c8203f076a451526
92326d8976 [rpc] add send method (Sjors Provoost)
2c2a1445dc [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0fd59 [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost)
Pull request description:
`walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have:
* manual coin selection
* outputting a PSBT (it was controversial to add this, see #18201)
* create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`)
At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases.
This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format.
Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546).
For `bitcoin-cli` users, it tries to keep the simplest use case easy to use:
```sh
bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
```
This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address.
Depends on:
- [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`)
- [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`)
- [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`)
ACKs for top commit:
meshcollider:
Light re-utACK 92326d8976
achow101:
ACK 92326d8976 Reviewed code and test, ran tests.
kallewoof:
utACK 92326d8976
Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f
Use object returned from add_p2p_connection to refer to
p2ps. Add a test class attribute if it needs to be used across
many methods. Don't use the p2p property.
Adds two new features to MiniWallet:
* The fee rate is irrelevant sometimes, so just set an arbitrary default
* The utxo to spend needs to be selected manually sometimes
fa188c9c59 test: Use MiniWalet in p2p_feefilter (MarcoFalke)
fa39c62eb7 test: inline hashToHex (MarcoFalke)
Pull request description:
This introduces a minimalistic test wallet, which can be used as a drop in replacement for the Bitcoin Core wallet to create dummy transactions with a given fee rate.
ACKs for top commit:
jnewbery:
utACK fa188c9c59
Tree-SHA512: 0aad9cb14eea4f0055bd6a47cc8c8f82a16941b152598c3bf1e083aae84cca4ffa23f0b854a362a68be1b917deba1b5ec7c0207b63b0805d747ba9a7d1d82efe
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).
Tests are updated to be started with -wallet= if they need the default
wallet.
Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.
7bf6dfbb48 wallet: Remove path checking code from bitcoin-wallet tool (Russell Yanofsky)
77d5bb72b8 wallet: Remove path checking code from createwallet RPC (Russell Yanofsky)
a987438e9d wallet: Remove path checking code from loadwallet RPC (Russell Yanofsky)
8b5e7297c0 refactor: Pass wallet database into CWallet::Create (Russell Yanofsky)
3c815cfe54 wallet: Remove Verify and IsLoaded methods (Russell Yanofsky)
0d94e60625 refactor: Use DatabaseStatus and DatabaseOptions types (Russell Yanofsky)
b5b414151a wallet: Add MakeDatabase function (Russell Yanofsky)
288b4ffb6b Remove WalletLocation class (Russell Yanofsky)
Pull request description:
Get rid of file path handling in wallet application code and move it down to database layer.
There is no change in behavior except for some changed error messages.
Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.
ACKs for top commit:
achow101:
ACK 7bf6dfbb48
meshcollider:
Code re-review and functional test run ACK 7bf6dfbb48
Tree-SHA512: 23ad18324c9e8947f0cf88a3734c2e9fb25536b2cb4d552cf5d1a4ade320fbffb73bb2d1b3a99585c11630aa7092e0fcfc2dd4fe65b91e3a54161433a5cd13cb
581b343d5b Add in/out connections to cli -getinfo (Jon Atack)
d9cc13e88d UNIX_EPOCH_TIME fixup in rpc getnettotals (Jon Atack)
1ab49b81cf Add in/out connections to rpc getnetworkinfo (Jon Atack)
Pull request description:
This is basic info that is present in the GUI that I've been wishing to have exposed via the RPC and CLI without needing a bash workaround or script. For human users it would also be useful to have it in `-getinfo`.
`bitcoin-cli getnetworkinfo`
```
"connections": 15,
"connections_in": 6,
"connections_out": 9,
```
`bitcoin-cli -getinfo`
```
"connections": {
"in": 6,
"out": 9,
"total": 15
},
```
Update the tests, RPC help, and release notes for the changes. Also fixup the `getnettotals` timemillis help while touching `rpc/net.cpp`.
-----
Reviewers can manually test this PR by [building from source](https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests), launching bitcoind, and then running `bitcoin-cli -getinfo`, `bitcoin-cli getnetworkinfo`, `bitcoin-cli help getnetworkinfo`, and `bitcoin-cli help getnettotals` (for the UNIX epoch time change).
ACKs for top commit:
eriknylund:
> tACK [581b343](581b343d5b) on master at [a0a422c](a0a422c34c), ran unit & functional tests and and confirmed changes on an existing datadir ✌️
benthecarman:
tACK `581b343`
willcl-ark:
tACK for 581b343d5b, this time rebased onto master at 862fde88be.
shesek:
tACK `581b343`. This provides what I needed, thanks!
n-thumann:
tACK 581b343 on master at a0a422c, ran unit & functional tests and and confirmed changes on an existing datadir ✌️
Tree-SHA512: 08dd3ac8fefae401bd8253ff3ac027603c528eeccba53cedcb127771316173a7052fce44af8fa33ac98ebc4cf2a2b11cdefd949995d55e9b9a5942b876d00dc5
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.
This commit does not change behavior except for error messages which now
include more complete information.
It's almost impossible to read bytes literals in code, so replace them
with the hex string literal and then convert them to a bytes object
using bytes.fromhex().
6de9429087 qa: Changes v0.17.1 to v0.17.2 (nthumann)
Pull request description:
As of 0374e821bd v0.17.2 is downloaded instead of v0.17.1 for functional testing. This causes `test/functional/feature_backwards_compatibility.py` to fail, because it [requires](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_backwards_compatibility.py#L57) v0.17.1.
Steps to reproduce:
Run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2`. It cannot be downloaded at all because the sha256sum is missing [here](c1e0c2ad3b/test/get_previous_releases.py (L23)).
Or adjust the command and run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`, then run `test/functional/test_runner.py feature_backwards_compatibility`. It´ll fail because the test is missing v0.17.1.
This PR changes v0.17.1 to v0.17.2 in this test and in a few comments.
ACKs for top commit:
laanwj:
ACK 6de9429087
fanquake:
ACK 6de9429087 - looks correct. Surprised this wasn't caught/part of #19813. In future you could add any explanations & extra info as part of your commit message as well (even though PR descriptions are included as part of the merge).
Tree-SHA512: bbe50c4fd5c1aedd6dc1cdc3d93ef9005db1c67adca3f263b6b0d869c40b495a3221e706c9389fedea4748e31911dbd591062f60ce9836e58099fbdd9515b4d9
fa1cd9e1dd test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)
Pull request description:
This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.
ACKs for top commit:
laanwj:
Code review ACK fa1cd9e1dd
hebasto:
ACK fa1cd9e1dd, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612
jnewbery:
utACK fb56d37612
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
36ec9801a4 test: Add chacha20 test vectors in muhash (Fabian Jahr)
0e2b400fea test: Add basic Python/C++ Muhash implementation parity unit test (Fabian Jahr)
b85543cb73 test: Add Python MuHash3072 implementation to test framework (Pieter Wuille)
ab30cece0e test: Move modinv to util and add unit test (Fabian Jahr)
Pull request description:
This is the second in a [series of pull requests](https://github.com/bitcoin/bitcoin/pull/18000) to implement an Index for UTXO set statistics.
This pull request adds a Python implementation of Muhash3072, a homomorphic hashing algorithm to be used for hashing the UTXO set. The Python implementation can then be used to compare behavior with the C++ version.
ACKs for top commit:
jnewbery:
utACK 36ec9801a
laanwj:
Code review ACK 36ec9801a4
Tree-SHA512: a3519c6e11031174f1ae71ecd8bcc7f3be42d7fc9c84c77f2fbea7cfc5ad54fcbe10b55116ad8d9a52ac5d675640eefed3bf260c58a02f2bf3bc0d8ec208baa6
3340dbadd3 Remove -zapwallettxes (Andrew Chow)
Pull request description:
It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.
Alternative to #19700
ACKs for top commit:
meshcollider:
utACK 3340dbadd3
fanquake:
ACK 3340dbadd3 - remaining manpage references will get cleaned up pre-release.
Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
7356292e1d Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983 Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600c Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)
Pull request description:
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion:
1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)
ACKs for top commit:
laanwj:
code review ACK 7356292e1d
promag:
Code review ACK 7356292e1d.
Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
fa3d9ce325 rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc20 rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7 rpc: Remove unused return type from appendCommand (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK fa3d9ce325
promag:
Code review ACK fa3d9ce325.
Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
6d1f51343c [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)
Pull request description:
When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.
Note that when creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC.
See #7518 for historical background.
ACKs for top commit:
meshcollider:
Code review ACK 6d1f51343c
fjahr:
Code review ACK 6d1f51343c
Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
d841301010 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz)
1343c86c7c test: Update wait_until usage in tests not to use the one from utils (Seleme Topuz)
Pull request description:
Replace global (from [test_framework/util.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L228)) `wait_until()` usages with the ones provided by `BitcoinTestFramework` and `P2PInterface` classes.
The motivation behind this change is that the `util.wait_until()` expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. `BitcoinTestFramework` offers a `wait_until()` which has an understandable amount of default `timeout` and a shared `timeout_factor`. Moreover, on top of these, `mininode.wait_until()` also has a shared lock.
closes#19080
ACKs for top commit:
MarcoFalke:
ACK d841301010🦆
kallewoof:
utACK d841301010
Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88
fafc9d5af4 test: Fix intermittent issue in wallet_bumpfee (MarcoFalke)
fa347b2f25 test: Select at least the fee in wallet_bumpfee to avoid negative amounts (MarcoFalke)
Pull request description:
With a "dirty" mempool a transaction might fail to be accepted intermittently. For example,
* https://travis-ci.org/github/bitcoin-core/gui/jobs/719916499#L6773 Fails acceptance
* https://travis-ci.org/github/bitcoin-core/gui/jobs/719916499#L6954 Test fails
Fix the issue by clearing the mempool between subtests
ACKs for top commit:
promag:
Code review ACK fafc9d5af4.
Tree-SHA512: 23fb6decb6343d19eafddcbdb7da0551f6be11325d1c97c30e563944000aeb02bcc4b24904d204b132c093dc1acf28445fa1fd08bfe8d8b52ddd1de51c33eeb6
d5800da519 [test] Remove final references to mininode (John Newbery)
5e8df3312e test: resort imports (John Newbery)
85165d4332 scripted-diff: Rename mininode to p2p (John Newbery)
9e2897d020 scripted-diff: Rename mininode_lock to p2p_lock (John Newbery)
Pull request description:
New contributors are often confused by the terminology in the test framework, and what the difference between a _node_ and a _peer_ is. To summarize:
- a 'node' is a bitcoind instance. This is the thing whose behavior is being tested. Each bitcoind node is managed by a python `TestNode` object which is used to start/stop the node, manage the node's data directory, read state about the node (eg process status, log file), and interact with the node over different interfaces.
- one of the interfaces that we can use to interact with the node is the p2p interface. Each connection to a node using this interface is managed by a python `P2PInterface` or derived object (which is owned by the `TestNode` object). We can open zero, one or many p2p connections to each bitcoind node. The node sees these connections as 'peers'.
For historic reasons, the word 'mininode' has been used to refer to those p2p interface objects that we use to connect to the bitcoind node (the code was originally taken from the 'mini-node' branch of https://github.com/jgarzik/pynode/tree/mini-node). However that name has proved to be confusing for new contributors, so rename the remaining references.
ACKs for top commit:
amitiuttarwar:
ACK d5800da519
MarcoFalke:
ACK d5800da519🚞
Tree-SHA512: 2c46c2ac3c4278b6e3c647cfd8108428a41e80788fc4f0e386e5b0c47675bc687d94779496c09a3e5ea1319617295be10c422adeeff2d2bd68378e00e0eeb5de
5da96210fc doc: release note for getpeerinfo last_block/last_transaction (Jon Atack)
cfef5a2c98 test: rpc_net.py logging and test naming improvements (Jon Atack)
21c57bacda test: getpeerinfo last_block and last_transaction tests (Jon Atack)
8a560a7d57 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack)
02fbe3ae0b net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack)
Pull request description:
This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`.
This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in https://github.com/bitcoin/bitcoin/pull/19643#issuecomment-671093420.
Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549:
```text
<jonatack> i was specifically trying to observe and figure out how to test https://github.com/bitcoin/bitcoin/issues/19500
<jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail
<jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
<jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
<sipa> jonatack: nope; i suspect just nobody ever added it
<jonatack> sipa: thanks. will propose.
```
The last commit is optional, but I think it would be good to have logging in `rpc_net.py`.
ACKs for top commit:
jnewbery:
Code review ACK 5da96210fc
theStack:
Code Review ACK 5da96210fc
darosior:
ACK 5da96210fc
Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70
5067c5acc3 [test] Add test for getblockheader verboseness (Torhte Butler)
Pull request description:
Improve test coverage by adding a test for getblockheader with verbose argument set to false.
ACKs for top commit:
theStack:
ACK 5067c5acc3
Tree-SHA512: e55593f1026a89dc7b796fa985b4cbcdb596e91d80d42dfb0660bda1692aaa35749ec29f9cd7032803f6225afb323f085df1ef6a9982de87be8e098f7253cdd5
124e1ee134 doc: Add release notes for getindexinfo RPC (Fabian Jahr)
c447b09458 test: Add tests for getindexinfo RPC (Fabian Jahr)
667bc7a7f7 rpc: Add getindexinfo RPC (Fabian Jahr)
Pull request description:
As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (#14053, #18000) that can give a quick overview without the user having to parse the logs.
Feature summary:
- Adds new RPC `listindices` (placed in Util section)
- That RPC only lists the actively running indices
- For each index it gives the name, whether it is synced and up to which block height it is synced
ACKs for top commit:
laanwj:
Re-ACK 124e1ee134
jonatack:
Code review re-ACK 124e1ee per `git range-diff a57af89 47a5372 124e1ee` no change since my last re-ACK, rebase only
Tree-SHA512: 3b7174c87951e6457fef099f530337803906baf32fb64261410b8def2c0917853d6a1bf3059cd590b1cc1523608f8916dafb327a431d27ecbf8d7454406b5b35
It's also clearer to have `no_version_disconnect_node` send a message
other than version or verack in order to reach the peer discouragement
threshold.
7f13dfb587 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf69 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587
achow101:
ACK 7f13dfb587
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
9e7894357e test: speedup p2p_feefilter.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
fe3f0cc44e test: use wait_until for invs matching in p2p_feefilter.py (Sebastian Falbesoner)
6d941923c3 test: add logging for p2p_feefilter.py (Sebastian Falbesoner)
Pull request description:
This PR gives some love to the functional test `p2p_feefilter.py` by introducing the following changes:
* add missing log messages for the `test_feefilter` subtest (the main one)
* deduplicate code by using the utility function `wait_until` (already using the [recently introduced version](12410b1feb)) instead of a manual condition checking loop with `time.sleep`
* improve naming of the function `matchAllInvs` (more expressive name, snake_case) and moving it from global namespace to the connection class `FeefilterConn`
* speeding up the test significantly by the good ol' method of activating immediate tx relay (as seen on e.g. https://github.com/bitcoin/bitcoin/pull/17121, https://github.com/bitcoin/bitcoin/pull/17124, https://github.com/bitcoin/bitcoin/pull/17340, https://github.com/bitcoin/bitcoin/pull/17362, ...):
```
master branch:
$ time ./p2p_feefilter.py
...
real 0m39.367s
user 0m1.227s
sys 0m0.571s
PR branch:
$ time ./p2p_feefilter.py
...
real 0m9.386s
user 0m1.120s
sys 0m0.577s
```
ACKs for top commit:
instagibbs:
code review ACK 9e7894357e
jonatack:
re-ACK 9e78943 per `git range-diff 3ab2582 ea74a3c 9e78943`
Tree-SHA512: fe21c1c5413df9165fea916b5d5f609d3ba33e7b5c3364b38eb824fcc55d9e6abddf27116cbc0b325913d451a73c44542040fb916aec9c46f805c6e12f6f10cf
fa330ec2fe test: Remove confusing and broken use of wait_until global (MarcoFalke)
fa6583c30b ci: Set increased --timeout-factor by default (MarcoFalke)
Pull request description:
Assuming that tests don't have a logic error or race, setting a high timeout should not cause any issues. The tests will still pass just as fast in the fastest case, but it allows for some buffer in case of slow disks or otherwise starved ci machines.
Fixes#19729
ACKs for top commit:
hebasto:
ACK fa330ec2fe, I have reviewed the code, and it looks OK, I agree it can be merged.
Tree-SHA512: 3da80ee008c7b08bab5fdaf7804d57c79d6fed49a7d37b9c54fc89756659fcb9981fd10afc4d07bd90d93c1699fd410a0110a3cd34d016873759d114ce3cd82d
