3bd67ba5a4 Test addr response caching (Gleb Naumenko)
cf1569e074 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43 Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)
Pull request description:
This is a very simple code change with a big p2p privacy benefit.
It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.
Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
I even have a script for that. It is totally doable within couple minutes.
Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.
I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!
I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.
ACKs for top commit:
jnewbery:
reACK 3bd67ba5a4
promag:
Code review ACK 3bd67ba5a4.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
Introduced in ca10a03add then erroneously removed in
8d8099e97a. The restored line is how we are
checking that the node will still re-request a specific txid given a witness-related failure.
decimal.InvalidOperation is a special case of a float parsing error, which
presumably should be handled in the same way as a general parsing error,
rather than blow up.
Alternatives include: logging the error, or re-raising with more information.
Example log output:
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 603, in sync_all
self.sync_blocks(nodes)
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 568, in sync_blocks
best_hash = [x.getbestblockhash() for x in rpc_connections]
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 568, in <listcomp>
best_hash = [x.getbestblockhash() for x in rpc_connections]
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_node.py", line 571, in __call__
return self.cli.send_cli(self.command, *args, **kwargs)
File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_node.py", line 639, in send_cli
return json.loads(cli_stdout, parse_float=decimal.Decimal)
File "/usr/lib64/python3.6/json/__init__.py", line 367, in loads
return cls(**kw).decode(s)
File "/usr/lib64/python3.6/json/decoder.py", line 339, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib64/python3.6/json/decoder.py", line 355, in raw_decode
obj, end = self.scan_once(s, idx)
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
This is a more narrowly-construed wait which eliminates the possibility of the
wait being triggered by other messages.
Co-authored-by: Billy Garrison <billygarrison.btc@gmail.com>
82dee87933 test: test decodepsbt fee calculation (count input value only once per UTXO) (Sebastian Falbesoner)
Pull request description:
Fixes#19523, adding a simple test to `rpc_psbt.py` that checks that the decodepsbt fee matches the one given by the wallet (`walletcreatefundedpsbt`). This is in particular important for PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type set.
Example test run after reverting commit 75122780e2 ("Increment input value sum only once per UTXO in decodepsbt"):
```
$ test/functional/rpc_psbt.py
2020-07-26T11:31:44.862000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__sutcd4y
20.00007580
2020-07-26T11:31:47.073000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/test_framework.py", line 118, in main
self.run_test()
File "test/functional/rpc_psbt.py", line 166, in run_test
assert_equal(decoded['fee'], created_psbt['fee'])
File "/home/honeybadger/buidl/bitcoin_thestack/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(20.00007580 == 0.00007580)
2020-07-26T11:31:47.125000Z TestFramework (INFO): Stopping nodes
......
```
ACKs for top commit:
achow101:
ACK 82dee87933
Tree-SHA512: 296b8a701f851d482ef6200c6cbf0cf0257a79a828ac6dbc39b05d8c2d839c6fdb9d3f5a084015295cfa3eac7c11faa2f2d52e619c11627b04c75150eead8330
2c6a02e024 Clean message_count and last_message (Troy Giorshev)
Pull request description:
From #19580
This PR changes comments to clarify the intended usage of `message_count` and `last_message`. Additionally it changes the only usage of `message_count` to use `last_message` instead, bringing the code into alignment with the intended usage.
Note: Now `message_count` is completely unused. However, it is ready to be used (i.e. the supporting code works) and likely will be used in some test in the future.
ACKs for top commit:
jnewbery:
utACK 2c6a02e024
Tree-SHA512: 07c7684c9586de4f845e10d7aac36c1aab9fb56b409949c1c70d5ca705bc3971ca7d5943245a0472def4efd7b4e1c5dad2f713db5ead8fca08404daf4891e98b
74507ce71e walletdb: Remove BerkeleyBatch friend class from BerkeleyDatabase (Andrew Chow)
00f0041351 No need to check for duplicate fileids in all dbenvs (Andrew Chow)
d86efab370 walletdb: Move Db->open to BerkeleyDatabase::Open (Andrew Chow)
4fe4b3bf1b walletdb: track database file use as m_refcount within BerkeleyDatabase (Andrew Chow)
65fb8807ac Combine BerkeleyEnvironment::Verify into BerkeleyDatabase::Verify (Andrew Chow)
Pull request description:
`BerkeleyBatch` and `BerkeleyDatabase` are kind of messy. The goal of this is to clean up them up so that they are logically separated.
`BerkeleyBatch` currently handles the creation of the `BerkeleyDatabase`'s `Db` handle. This is instead moved into `BerkeleyDatabase` and is called by `BerkeleyBatch`.
Instead of having `BerkeleyEnvironment` track each database's usage, have `BerkeleyDatabase` track this usage itself with the `m_refcount` variable that is present in `WalletDatabase`.
Lastly, instead of having each `BerkeleyEnvironment` store the fileids of the databases open in it, have a global `g_fileids` to track those fileids. We were already checking fileid uniqueness globally (by checking the fileids in every environment when opening a database) so it's cleaner to do this with a global variable.
All of these changes allow us to make `BerkeleyBatch` and `BerkeleyDatabase` no longer be friend classes.
The diff of this PR is currently the same as in ##18971
Requires #19334
ACKs for top commit:
laanwj:
Code review ACK 74507ce71e
ryanofsky:
Code review ACK 74507ce71e. No changes since last review other than rebase
Tree-SHA512: 845d84ee1a470e2bf5d2e2e3d7738183d8ce43ddd06a0bbd57edecf5779b2f55d70728b1b57f5daab0f078650a8d60c3e19dc30b75b36e7aa952ce268399d5f6
This commit clarifies the intended usage of message_count and
last_message. Additionally it changes the only usage of message_count
to using last_message instead, bringing the code further along the
intended usage.
Checks that the RPC decodepsbt calculates the fee correctly, in particular for
PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type
set. Before commit 75122780e2 ("Increment input
value sum only once per UTXO in decodepsbt") the values for those inputs were
double counted.
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c5🌾
jnewbery:
utACK 9c69cfe4c5
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
2aac093a3d test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b12 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e net: Add -networkactive option (Hennadii Stepanov)
Pull request description:
Some Bitcoin Core activity is completely local (offline), e.g., reindexing.
The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`.
This was done while reviewing #16981.
ACKs for top commit:
MarcoFalke:
re-ACK 2aac093a3d🏠
LarryRuane:
ACK 2aac093a3d
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
0a4f1422cd Further improve comments around recentRejects (Suhas Daftuar)
0e20cfedb7 Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar)
cacd85209e test: Use wtxid relay generally in functional tests (Fabian Jahr)
8d8099e97a test: Add tests for wtxid tx relay in segwit test (Fabian Jahr)
9a5392fdf6 test: Update test framework p2p protocol version to 70016 (Fabian Jahr)
dd78d1d641 Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar)
4eb515574e Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar)
97141ca442 Delay getdata requests from peers using txid-based relay (Suhas Daftuar)
46d78d47de Add p2p message "wtxidrelay" (Suhas Daftuar)
2d282e0cba ignore non-wtxidrelay compliant invs (Anthony Towns)
ac88e2eb61 Add support for tx-relay via wtxid (Suhas Daftuar)
8e68fc246d Add wtxids to recentRejects instead of txids (Suhas Daftuar)
144c385820 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar)
85c78d54af Add wtxid-index to orphan map (Suhas Daftuar)
08b39955ec Add a wtxid-index to mapRelay (Suhas Daftuar)
60f0acda71 Just pass a hash to AddInventoryKnown (Suhas Daftuar)
c7eb6b4f1f Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar)
2b4b90aa8f Add a wtxid-index to the mempool (Suhas Daftuar)
Pull request description:
Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.
We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.
As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).
Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.
Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.
To do items:
- [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
- [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.
ACKs for top commit:
naumenkogs:
utACK 0a4f1422cd
laanwj:
utACK 0a4f1422cd
Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
12410b1feb test: fix intermittent p2p_ibd_txrelay race, add test_framework.py#wait_until (Jon Atack)
Pull request description:
To fix these intermittent failures in Travis CI.
```
162/163 - p2p_ibd_txrelay.py failed, Duration: 2 s
stdout:
2020-07-19T05:44:17.213000Z TestFramework (INFO):
Check that nodes set minfilter to MAX_MONEY while still in IBD
2020-07-19T05:44:17.216000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/p2p_ibd_txrelay.py", line 30, in run_test
assert_equal(conn_info['minfeefilter'], MAX_FEE_FILTER)
File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/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(0E-8 == 0.09170997)
2020-07-19T05:44:17.293000Z TestFramework (INFO): Stopping nodes
```
At Marco's suggestion, cherry-picked part of #19134 to nicely simplify using `wait_until`.
ACKs for top commit:
vasild:
ACK 12410b1fe
Tree-SHA512: 615f509883682fd693e578b259cba35a9fa0bc519f1394e88c857e8b0650bfec5397bfa856cfa9e6d5ef81d0ee6ad02e4ad2b0eb0bd530b4c281cbe3e663790b
Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.
Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
cb31ee01b4 [test] feefilter during and after IBD (gzhao408)
Pull request description:
This is a followup to #19204 which uses `minfeefilter=MAX_MONEY` to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was [missing](https://github.com/bitcoin/bitcoin/pull/19204#issuecomment-644040070) a functional test.
ACKs for top commit:
jnewbery:
utACK cb31ee01b4
Tree-SHA512: a9effc8193fa95fb42a2f9c66b258cc7b0941fc04c1ce3a6092f4426c9bfc7e72f702aca559b3e30e90652497f411f22fae3cf5cdb6cfd6ef6d37fed712cda67
e80259f197 Additionally treat Tx.nVersion as unsigned in joinpsbts (Matt Corallo)
970de70bdd Dump transaction version as an unsigned integer in RPC/TxToUniv (Matt Corallo)
Pull request description:
Consensus-wise we already treat it as an unsigned integer (the
only rules around it are in CSV/locktime handling), but changing
the underlying data type means touching consensus code for a
simple cleanup change, which isn't really worth it.
See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299
ACKs for top commit:
sipa:
ACK e80259f197
practicalswift:
ACK e80259f197
ajtowns:
ACK e80259f197 code review -- checked all other uses of tx.nVersion treat it as unsigned (except for policy.cpp:IsStandard anyway), so looks good.
naumenkogs:
ACK e80259f
Tree-SHA512: 6760a2c77e24e9e1f79a336ca925f9bbca3a827ce02003c71d7f214b82ed3dea13fa7d9f87df9b9445cd58dff8b44a15571d821c876f22f8e5a372a014c9976b
fa108d6a75 test: update tests for peer discouragement (Jon Atack)
1a9f462caa gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a75
laanwj:
ACK fa108d6a75
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
7bb6f9bfdb [protocol] Remove unused GETHEADERS_VERSION (John Newbery)
37a934e6b3 [protocol] Remove unused CADDR_TIME_VERSION (John Newbery)
Pull request description:
These constants are no longer required and can be removed.
Additional code comments are added to explain CAddress serialization.
ACKs for top commit:
MarcoFalke:
ACK 7bb6f9bfdb already an improvement, but maybe getting rid of INIT_PROTO_VERSION here would be an even stronger improvement (can be done later)
jonatack:
ACK 7bb6f9bfdb
vasild:
ACK 7bb6f9bf
Tree-SHA512: 5382562c60fd677c86583754eca11aad3719064efe2e5ef4f307d693b583422ca8d385926c2582aaab899f502b151f2eb87a7ac23363b15f4fceaa06296f98e3
08fc6f6cfc [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)
Pull request description:
I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.
Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.
Salvaged from #18201.
ACKs for top commit:
fjahr:
Code review ACK 08fc6f6cfc
jonatack:
ACK 08fc6f6cfc
meshcollider:
Code review & functional test run ACK 08fc6f6cfc
Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
a66a7a1a70 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow)
Pull request description:
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.
ACKs for top commit:
hugohn:
tACK [a66a7a1](a66a7a1a70)
jonatack:
ACK a66a7a1a70
meshcollider:
Code review ACK a66a7a1a70
Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
fa73493930 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c19 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a61897 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)
Pull request description:
User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.
ACKs for top commit:
jnewbery:
utACK fa73493930
meshcollider:
utACK fa73493930
ryanofsky:
Code review ACK fa73493930. Just rebased since last review
Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c
Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to
the loadwallet RPC and maintain a dynamic list of wallets that should be loaded
on startup that also can be shared between bitcoind and bitcoin-qt.
fab5586122 doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab5586122
fjahr:
Code review ACK fab5586122
jonatack:
ACK fab5586122
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
41d55d3057 doc: getpeerinfo banscore deprecation release note (Jon Atack)
dd54e3796e test: getpeerinfo banscore deprecation test (Jon Atack)
8c7647b3fb rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack)
Pull request description:
Per https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592, this PR deprecates returning the `banscore` field in the `getpeerinfo` RPC, updates the help, adds a test, and updates the release notes. Related to #19464.
ACKs for top commit:
fanquake:
ACK 41d55d3057
Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc
40506bf93f test: Test gettxouttsetinfo hash_type option (Fabian Jahr)
f17a4d1c4d rpc: Add hash_type NONE to gettxoutsetinfo (Fabian Jahr)
a712cf6f68 rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) (Fabian Jahr)
605884ef21 refactor: Extract GetBogoSize function (Fabian Jahr)
Pull request description:
This is another intermediate part of the Coinstats Index (tracked in #18000).
Sjors suggested [here](https://github.com/bitcoin/bitcoin/pull/18000#issuecomment-641423019) that the part of the changes in #19145 that don't rely on the new `hash_type` muhash, i.e. that are for `hash_type=none`, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements.
Building the index with no UTXO set hash is still valuable because `gettxoutsetinfo` can still be used to audit the `total_amount` for example. By itself this PR is not a huge improvement, `hash_type=none` is speeding up `gettxoutsetinfo` by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer.
ACKs for top commit:
MarcoFalke:
ACK 40506bf93f 🖨
Sjors:
tACK 40506bf93f
Tree-SHA512: 3964c2b8eed427511b1aa9b2ef285dff27dc4d1537d72c3911e435b6e6b40912232da4acb3a09bd19a0372ddffa44103388d8a650169d95a4a727b970d210add
d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d1, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
When loading descriptor caches, we would accidentally reinitialize the
descriptor cache when seeing that one already exists. This should have
only been initializing the cache when one does not exist. However this
code itself is unnecessary as the act of looking up the cache to add to
it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a
multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case.
Another test case has also been added to check that loading a wallet
with only single key descriptors works.
84d295e513 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
4600479058 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e513 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e513. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e513
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
3a7e79478a test: retry when write to a socket fails on macOS (Ivan Metlushko)
8cf9d15b82 test: use pgrep for better compatibility (Ivan Metlushko)
Pull request description:
Rationale: a few minor changes to make experience of running tests on macOS a bit better
1.`pidof` is not available on BSD/macOS, while `pgrep` is present on BSD, Linux and macOS
2. Add retry as a workaround for a weird behavior when writing to a socket (https://bugs.python.org/issue33450). Stacktrace attached
Man pages:
https://www.freebsd.org/cgi/man.cgi?query=pgrep&apropos=0&sektion=1&manpath=FreeBSD+6.0-RELEASE&arch=default&format=htmlhttps://man7.org/linux/man-pages/man1/pgrep.1.html
Related to #19281
Stacktrace example:
```
...
33/161 - feature_abortnode.py failed, Duration: 63 s
stdout:
2020-06-11T10:46:43.947000Z TestFramework (INFO): Initializing test directory /var/folders/2q/d5w9zh614r7g5c8r74ln3g400000gq/T/test_runner_₿_🏃_20200611_174102/feature_abortnode_128
2020-06-11T10:46:45.199000Z TestFramework (INFO): Waiting for crash
2020-06-11T10:47:15.921000Z TestFramework (INFO): Node crashed - now verifying restart fails
2020-06-11T10:47:47.068000Z TestFramework (INFO): Stopping nodes
[node 1] Cleaning up leftover process
stderr:
Traceback (most recent call last):
File "/Users/xxx/Projects/bitcoin/test/functional/feature_abortnode.py", line 50, in <module>
AbortNodeTest().main()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
exit_code = self.shutdown()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 266, in shutdown
self.stop_nodes()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 515, in stop_nodes
node.stop_node(wait=wait)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_node.py", line 318, in stop_node
self.stop(wait=wait)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
self.__conn.request(method, path, postdata, headers)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1107, in request
self._send_request(method, url, body, headers)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1152, in _send_request
self.endheaders(body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1103, in endheaders
self._send_output(message_body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 936, in _send_output
self.send(message_body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 908, in send
self.sock.sendall(data)
OSError: [Errno 41] Protocol wrong type for socket
```
ACKs for top commit:
laanwj:
ACK 3a7e79478a
Tree-SHA512: fefbe40ce94ab29f18bbbed2a434194b1384ffa5279b1d04db7a3708e3dd422bd9e450f1db3f95a1a851fac5a626ab533c6ebcfd7ede96f8ccae9e6f3e9fff92
9b009fae6e qa: Test concurrent wallet loading (João Barbosa)
b9971ae585 wallet: Handle concurrent wallet loading (João Barbosa)
Pull request description:
This PR handles concurrent wallet loading.
This can be tested by running in parallel the following script a couple of times:
```sh
for i in {1..10}
do
src/bitcoin-cli -regtest loadwallet foo
src/bitcoin-cli -regtest unloadwallet foo
done
```
Eventually the error occurs:
```
error code: -4
error message:
Wallet already being loading.
```
For reference, loading and already loaded wallet gives:
```
error code: -4
error message:
Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.
```
Fixes#19232.
ACKs for top commit:
MarcoFalke:
Concept ACK 9b009fae6e I have not reviewed the code
hebasto:
ACK 9b009fae6e, tested on Linux Mint 20 (x86_64):
ryanofsky:
Code review good-but-not-ideal ACK 9b009fae6e
Tree-SHA512: 0ccd77b03c0926e4c4e51efb31e193b93cb4b9ffe8bac6bb018f7344c55dfd939b873b8cf5e657dca73e6202eb75aa672de2acb787cc133184b0b3b51e47b972
fa525e4d1c net: Avoid wasting inv traffic during IBD (MarcoFalke)
fa06d7e934 refactor: block import implies IsInitialBlockDownload (MarcoFalke)
faba65e696 Add ChainstateManager::ActiveChainstate (MarcoFalke)
fabf3d64ff test: Add FeeFilterRounder test (MarcoFalke)
Pull request description:
Tx-inv messages are ignored during IBD, so it would be nice if we told peers to not send them in the first place. Do that by sending two `feefilter` messages: One when the connection is made (and the node is in IBD), and another one when the node leaves IBD.
ACKs for top commit:
jamesob:
ACK fa525e4d1c ([`jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d`](https://github.com/jamesob/bitcoin/tree/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d))
naumenkogs:
utACK fa525e4
gzhao408:
ACK fa525e4d1c
jonatack:
re-ACK fa525e4 checked diff `git range-diff 19612ca fa8a66c fa525e4`, re-reviewed, ran tests, ran a custom p2p IBD behavior test at 9321e0f223.
hebasto:
re-ACK fa525e4d1c, only rebased since the [previous](https://github.com/bitcoin/bitcoin/pull/19204#pullrequestreview-429519667) review (verified with `git range-diff`).
Tree-SHA512: 2c22a5def9822396fca45d808b165b636f1143c4bdb2eaa5c7e977f1f18e8b10c86d4c180da488def38416cf3076a26de15014dfd4d86b2a7e5af88c74afb8eb
25dac9fa65 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm)
05227a3554 tests for bumpfee / estimate_modes (Karl-Johan Alm)
3404c1b753 policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm)
6fcf448430 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm)
b188d80c2d MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm)
5d1a411eb1 fees: add FeeModes doc helper function (Karl-Johan Alm)
91f6d2bc8f rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm)
69158b41fc added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm)
Pull request description:
This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators.
ACKs for top commit:
Sjors:
re-utACK 25dac9fa65: rebased, more fancy C++,
jonatack:
ACK 25dac9fa65 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted.
fjahr:
Code review ACK 25dac9fa65
Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33
If the socket is tearing down macOS will return EPROTOTYPE instead of EPIPE.
Because python doesn't handle this internally we have to do a workaround and retry the request.
See https://bugs.python.org/issue33450
56010f9256 test: hoist p2p values to test framework constants (Jon Atack)
75447f0893 test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack)
57960192a5 test: refactor test_large_inv() into 3 tests with common method (Jon Atack)
e2b21d8a59 test: add p2p_invalid_messages logging (Jon Atack)
9fa494dc09 net: update misbehavior logging for oversized messages (Jon Atack)
Pull request description:
...seen while reviewing #19264, #19252, #19304 and #19107:
in `net_processing.cpp`
- make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages
in `p2p_invalid_messages`
- add missing logging
- improve assertions/message sends, move cleanup disconnections outside the assertion scopes
- split a slowish 3-part test into 3 order-independent tests
- add a few p2p constants to the test framework
ACKs for top commit:
troygiorshev:
reACK 56010f9256
MarcoFalke:
ACK 56010f9256 🎛
Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
cc84460c16 test: move sync_blocks and sync_mempool functions to test_framework.py (Roy Shao)
Pull request description:
This PR moves `sync_blocks` and `sync_mempool` out from `test_framework/util.py` to `test_framework/test_framework.py` so they can take contextual information of test framework into account.
* Change all reference callers to call functions from `test_framework.py`
* Remove `**kwargs` which is not used
* Take into account of `timeout_factor` when respecting timeout in function implementations.
* Pass all tests by running `./test/functional/test_runner.py`
fixes#18930
ACKs for top commit:
MarcoFalke:
ACK cc84460c16 , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫
Tree-SHA512: a79b2a3fa842fc26a7aacb834bb2aea88b3049916c0b754e60002a77ce94bb5954e0ea3b436bf268e9295efb62d721dfef263a09339a55c684ac3fda388c275e
931dd47608 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29 [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b733 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd47608
jb55:
ACK 931dd47608
achow101:
ACK 931dd47608
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
22cb303cf0 rpc: add missing space in JSON parsing error message, update test (Jon Atack)
bf53ebef06 test: add multiwallet tests for bitcoin-cli -generate (Jon Atack)
4b859cfff9 cli: add multiwallet capability to GetNewAddress and -generate (Jon Atack)
18f93545a1 test: add tests for bitcoin-cli -generate (Jon Atack)
4818124137 cli: create bitcoin-cli -generate command (Jon Atack)
ff41a36900 cli: extract ParseResult() and ParseError() (Jon Atack)
f4185b26d9 cli: create GenerateToAddressRequestHandler class (Harris)
f7c65a3350 cli: create GetNewAddress() (Jon Atack)
9be7fd35c5 rpc: make generatetoaddress locals const (Jon Atack)
cb00510dba rpc: create rpc/mining.h, hoist default max tries values to constant (Jon Atack)
Pull request description:
This PR continues and completes the work begun in #17700 working on issue #16000 to create a client-side version of RPC `generate`.
Basically, `bitcoin-cli -generate` wraps calling `generatenewaddress` followed by `generatetoaddress [nblocks] [maxtries]` and prints the following:
```
$ bitcoin-cli -generate
{
"address": "bcrt1qn4aszr2y2xvpa70y675a76wsu70wlkwvdyyln6"
"blocks": [
"01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
]
}
$ bitcoin-cli -rpcwallet=wallet-name -generate 3 100
{
"address": "bcrt1q4cunfw0gnsj7g7e6mk0v0uuvvau9mwr09dj45l",
"blocks": [
"7a6650ca5e0c614992ee64fb148a7e5e022af842e4b6003f81abd8baf1e75136",
"01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
"3f8795ec40b1ad812b818c177680841be319a3f6753d4e32dc7dfb5bafe5d00e"
]
}
```
Help doc:
```
$ bitcoin-cli -h | grep -A5 "\-generate"
-generate
Generate blocks immediately, equivalent to RPC generatenewaddress
followed by RPC generatetoaddress. Optional positional arguments
are number of blocks to generate (default: 1) and maximum
iterations to try (default: 1000000), equivalent to RPC
generatetoaddress nblocks and maxtries arguments. Example:
bitcoin-cli -generate 4 1000
```
Quite a bit of test coverage turned out to be needed to cover the change and the different cases (arguments, multiwallet mode) and error-handling.
This PR also improves some things that working on these changes brought to light.
Credit to Harris Brakmić for the initial work in #17700.
ACKs for top commit:
adamjonas:
utACK 22cb303cf0
meshcollider:
utACK 22cb303cf0
Tree-SHA512: 94f67f632fe093d076f614e0ecff09ce7342ac6e424579200d5211a6615260e438d857861767fb788950ec6da0b26ef56dc8268c430012a3b3d4822b24ca6fbf
bc01f7ae05 doc: release note for rpc getaddressinfo removals (Jon Atack)
90e989390e rpc: getaddressinfo RPCResult fixup (Jon Atack)
a8507c99da rpc: remove deprecated getaddressinfo `labels: purpose` (Jon Atack)
645a8653c8 rpc: remove deprecated getaddressinfo `label` field (Jon Atack)
Pull request description:
These were deprecated in #17578 and #17585, with expected 0.21 removal notified in the 0.20 release notes.
```
- The `getaddressinfo` RPC has had its `label` field deprecated
(re-enable for this release using the configuration parameter
`-deprecatedrpc=label`). The `labels` field is altered from returning
JSON objects to returning a JSON array of label names (re-enable
previous behavior for this release using the configuration parameter
`-deprecatedrpc=labelspurpose`). Backwards compatibility using the
deprecated configuration parameters is expected to be dropped in the
0.21 release. (#17585, #17578)
```
ACKs for top commit:
Sjors:
utACK bc01f7a
adamjonas:
utACK bc01f7a
meshcollider:
utACK bc01f7ae05
Tree-SHA512: ae1af381e32c4c3bde8b061a56382838513a9a82c88767843cdeae3a2ab8aa7d8c2e66e106d2b31ea07d74bb80c191a2f842c9aaecc7c5438ad9a9bc66d1b251
e5327f947c [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)
79804fe24b [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost)
Pull request description:
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`.
Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`.
This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.
ACKs for top commit:
achow101:
ACK e5327f947c
meshcollider:
utACK e5327f947c
Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
951bca61d7 tests: feature_backwards_compatibility.py test 0.16 up/downgrade (Andrew Chow)
3a03a11e8c Skip hdKeypath of 'm' (Andrew Chow)
Pull request description:
Previously the seed was stored with keypath 'm' so we need to skip this as well when determining inactive seeds.
Fixes#19051
ACKs for top commit:
Sjors:
ACK 951bca61d7
instagibbs:
re-utACK 951bca61d7
ryanofsky:
Code review ACK 951bca61d7. No significant changes since last review, just updated comment and some test tweaks
Tree-SHA512: 930f77e7097c9cf4f1012e540bd2b1a72fd279262517f10c1531b2ad48c632ef95e0dd4edea81bcc3b3db306479d34e5e79e5d6c4ed31dfa4b77a4231436436e
- call disconnect_p2ps() outside of the assert_debug_log scopes
- send messages directly from the p2p conn rather than via nodes[0].p2p
- add an assertion
80d4423f99 Test buffered valid message (Troy Giorshev)
Pull request description:
This PR is a tweak of #19302. This sends a valid message.
Additionally, this test includes logging in the same vein as #19272.
ACKs for top commit:
MarcoFalke:
tested ACK 80d4423f99 (added an assert(false) to observe deterministic coverage) 🌦
gzhao408:
ACK 80d4423f99👊
Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8
A message can be broken across two buffers, with the split inside its
header. Usually this will occur when sending many messages, such that
the first buffer fills.
This test uses the RPC to verify that the message is actually being
received in two pieces.
There is a very rare chance of a race condition where the test framework
sends a message in between the two halves of the message under test. In
this case the peer will almost certainly disconnect and the test will
fail. An assert has been added to help debugging that rare case.
fa195d4eba test: Add missing sync_blocks (MarcoFalke)
Pull request description:
Bitcoin Core does not sort block and tx announcements for other peers, so generating 100 blocks and then sending out a transaction might reject it if it arrives too early. (non-final)
Fix that by syncing the blocks first.
Fix#19265Fix#19311
ACKs for top commit:
Sjors:
utACK fa195d4eba: sounds plausible
Tree-SHA512: fdc46aed59595e4189509e71bd4a3607a93893933cc01d806cec2ee7701d54d7422c5f22dd83b81ddb021f9113b3119a688fdd8cf8a6474fc12fea422aedd064
9a40cfc558 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1 [test] logging and style followups for bloomfilter tests (gzhao408)
Pull request description:
Followup to #19083 which adds bloomfilter-related tests.
1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)
ACKs for top commit:
jonatack:
Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
MarcoFalke:
ACK 9a40cfc558🐂
Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
-Waiting is important to avoid race conditions,
especially if testing peer info through rpc later.
-Wait for mininodes to be disconnected only, even
though it's more complex, because we may still want
to be connected to test nodes.
-Use peer to refer to mininodes instead of node
because they are not bitcoind nodes.
-Use log.debug for logs that give helpful but
not super necessary information.
-Adhere to style guidelines (newlines, capitalization).
16d4b3fd6d test: mempool.dat compatibility between versions (Ivan Metlushko)
Pull request description:
Rationale: Verify mempool.dat compatibility between versions
The format of mempool.dat has been changed in #18038
The tests verifies the fix made in #18807 and ensures that the file format is compatible between current version and v0.19.1
The test verifies both backward and forward compatibility.
This PR also adds a log when we fail to add a tx loaded from mempool.dat.
It was useful when debugging this test and could be potentially useful to debug other scenarios as well.
Closes#19037
ACKs for top commit:
Sjors:
tACK 16d4b3fd6d
Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490
62068381a3 [tests] Make mininode_lock non-reentrant (John Newbery)
c67c1f2c03 [tests] Don't call super twice in P2PTxInvStore.on_inv() (John Newbery)
9d80762fa0 [tests] Don't acquire mininode_lock twice in wait_for_broadcast() (John Newbery)
edae6075aa [tests] Only acquire lock once in p2p_compactblocks.py (John Newbery)
Pull request description:
There's no need for mininode_lock to be reentrant.
Use a simpler non-recursive lock.
ACKs for top commit:
MarcoFalke:
ACK 62068381a3😃
jonatack:
ACK 62068381a3
Tree-SHA512: dcbc19e6c986970051705789be0ff7bec70c69cf76d5b468c2ba4cb732883ad512b1de5c3206c2eca41fa3f1c4806999df4cabbf67fc3c463bb817458e59a19c
3a10d935ac [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430c65 [test] test disconnect for filterclear (gzhao408)
1c6b787e03 [netprocessing] disconnect node that sends filterclear (gzhao408)
Pull request description:
Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.
Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](19e919217e/src/net_processing.cpp (L2218)), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204).
Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.
ACKs for top commit:
jnewbery:
Code review ACK 3a10d935ac
naumenkogs:
utACK 3a10d93
gillichu:
tested ACK: quick test_runner on macOS [`3a10d93`](3a10d935ac)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
af2a145e57 Refactor resource exhaustion test (Troy Giorshev)
5c4648d17b Fix "invalid message size" test (Troy Giorshev)
ff1e7b8844 Move size limits to module-global (Troy Giorshev)
57890abf2c Remove two unneeded tests (Troy Giorshev)
Pull request description:
This PR touches only the p2p_invalid_messages.py functional test module. There are two main goals accomplished here. First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons. Second, it refactors the file into a single consistent style. This file appears to have originally had two authors, with different styles and some test duplication.
It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and [AltNet](https://github.com/bitcoin/bitcoin/issues/18989) changes.
This should probably go in ahead of #19107, but the two are not strictly related.
ACKs for top commit:
jnewbery:
ACK af2a145e57
MarcoFalke:
re-ACK af2a145e57🍦
Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
45eff751c6 Add functional test for P2P eviction logic of inbound peers (Martin Zumsande)
Pull request description:
This adds a functional test for the eviction logic for inbound peers, which is triggered when the number of maximum connections is exceeded.
The functional test covers eviction protection for peers that have sent us blocks or txns recently, or that have faster pings. I couldn't find a way to test the logic of `CConnman::AttemptToEvictConnection` that is based on netgroup (see #14210 for related discussion)
Fixes#16660 (at least partially).
[Edit: Earlier, this PR also contained a unit test, which was removed after the discussion]
ACKs for top commit:
jonatack:
ACK 45eff751c6
naumenkogs:
Tested ACK 45eff75
fjahr:
re-ACK 45eff751c6
andrewtoth:
re-ACK 45eff751c6
Tree-SHA512: 177208ab6f30dc62da1cc5f51e654f7c9770d8c6b42aca6ae7ecb30e29d3096e04d75739578e7d149a0f29dd92652b4a707e93c0f1be8aa7ed315e6ec3ab07a4
fadf6bd04f refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0 wallet: Make RPC help compile-time static (MarcoFalke)
Pull request description:
Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.
The fix is split into two commits:
* First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test.
* Second, a commit that removes the complicated and confusing code.
ACKs for top commit:
achow101:
re-ACK fadf6bd04f
promag:
Tested ACK fadf6bd04f.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
dca73941eb scripted-diff: rename node to peer for mininodes (gzhao408)
0474ea25af [test] fix race conditions and test in p2p_filter (gzhao408)
4ef80f0827 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408)
497a619386 [test] add BIP 37 test for node with fRelay=false (gzhao408)
e8acc60156 [test] add mempool msg test for node with bloomfilter enabled (gzhao408)
Pull request description:
This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_:
1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)).
2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`.
3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled.
4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
5. Fixes race conditions in p2p_filter.py
ACKs for top commit:
MarcoFalke:
ACK dca73941eb only changes is restoring accidentally deleted test 🍮
jonatack:
ACK dca73941eb modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.
Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3
3a83a01694 [tests] move generate_wif_key to wallet_util.py (John Newbery)
b216b0b71f [tests] sort imports in rpc_createmultisig.py (John Newbery)
e38081846d Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery)
Pull request description:
generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.
This fixes the circular dependency issue in #17977
ACKs for top commit:
MarcoFalke:
ACK 3a83a01694🍪
Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
fa98e10d5e test: Remove leftover comment in mining_basic (MarcoFalke)
faedb50d89 test: pep-8 mining_basic (MarcoFalke)
Pull request description:
Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing
ACKs for top commit:
adamjonas:
code review ACK fa98e10
Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40
-A node with bloomfilters disabled should disconnect peers that send
msg_mempool, msg_filterload, or msg_filteradd.
-Renamed the test because it now has a wider scope and msg_mempool's
actual functionality makes more sense for p2p_filter.py.
-msg_mempool is currently only tested with bloomfilter disabled
(node is disconnected) in p2p_mempool.py
-msg_mempool should get mempool txns in response when bloomfilter
is enabled
-edit test that doesn't test msg_mempool as intended
34645c4dd0 Test txinwitness is accessible on coinbase vin (Rod Vagg)
3e4421070a Expose txinwitness for coinbase in JSON form (Rod Vagg)
Pull request description:
## Rationale
The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.
Without the nonce you can't:
1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`)
I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.
## What
This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.
## Examples
Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7
```json
"vin": [
{
"coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
"txinwitness": [
"0000000000000000000000000000000000000000000000000000000000000000"
],
"sequence": 4294967295
}
],
...
```
Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731
```json
"vin": [
{
"coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
"txinwitness": [
"5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
],
"sequence": 4294967295
}
],
...
```
## Alternatives
This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`).
## Tests
This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.
ACKs for top commit:
MarcoFalke:
ACK 34645c4dd0
Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85
34e641a564 test: Remove unnecessary disconnect_nodes call in rpc_psbt.py (Danny Lee)
e6e7abd51a test: remove redundant two-way disconnect_nodes calls (Danny Lee)
a9bd1f9adf test: warn if nodes not connected before disconnect_nodes (Danny Lee)
Pull request description:
There's no harm in calling `disconnect_nodes` for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.
In addition, since `disconnect_nodes` works bidirectionally, I removed all instances of this pattern:
```
disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)
```
ACKs for top commit:
MarcoFalke:
review ACK 34e641a564👔
amitiuttarwar:
ACK 34e641a564. Thanks for this test improvement!
Tree-SHA512: 344855ceb46c012d43c13d7c09f44d32dcb7645706d10ae1e4645d9edca54c6c6c13fee26b79480755cdfcdf39b4b5770b36bb03ce71ba002d5be8a27fe008af
This is a simple refactor of the specified test. It is now brought in
line with the rest of the tests in the module. This should make things
easier to debug, as all of the tests are now grouped together at the
top.
This test originally made a message with an invalid stated length, and
an invalid checksum. This was because only the header was changed, but
the checksum stayed the same. This was fine for now because we check
the header first to see if it has a valid stated size, and we disconnect
if it does not, so we never end up checking for the checksum. If this
behavior was to change, this test would become a problem. (Indeed I
discovered this when playing around with this behavior). By instead
creating a message with an oversized payload from the start, we create a
message with an invalid stated length but a valid checksum, as intended.
Additionally, this takes advantage to the newly module-global
VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB,
but at the moment when receiving a message we check both, so this makes
the test tighter.
As well, this renames those variables to match PEP8 and this clears up
the comment relating to VALID_DATA_LIMIT.
Admittedly, this commit is mainly to make the following ones cleaner.
Test 1 is a duplicate of test_size() later in the file. Inexplicably,
this test does not work on macOS, whereas test_size() does.
Test 2 is problematic for two reasons. First, it always fails with an
invalid checksum, which is probably not what was intended. Second, it's
not defined at this layer what the behavior should be. Hypothetically,
if this test was fixed so that it gave messages with valid checksums,
then the message would pass successfully thought the network layer and
fail only in the processing layer. A priori the network layer has no
idea what the size of a message "actually" is.
The "Why does behavior change at 78 bytes" is because of the following:
print(len(node.p2p.build_message(msg))) # 125
=> Payload size = 125 - 24 = 101
If we take 77 bytes, then there are 101 - 77 = 24 left
That's exactly the size of a header
So, bitcoind deserializes the header and rejects it for some other reason
(Almost always an invalid size (too large))
But, if we take 78 bytes, then there are 101 - 78 = 23 left
That's not enough to fill a header, so the socket stays open waiting for
more data. That's why we sometimes have to push additional data in
order for the peer to disconnect.
Additionally, both of these tests use the "conn" variable. For fun, go
look at where it's declared. (Hint: test_large_inv(). Don't we all
love python's idea of scope?)
7daffc6a90 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
fa1433ac1b rpc: Remove special case for unknown service flags (MarcoFalke)
Pull request description:
The special case to return a bit as an integer is clumsy and undocumented. Probably also irrelevant because there shouldn't currently be a non-misbehaving client that connects to Bitcoin Core and advertises an unknown service flag.
Thus, simply remove the code.
ACKs for top commit:
laanwj:
ACK fa1433ac1b
Tree-SHA512: 942de6a577a9ee076ce12c92be121617640d53ee8c3424064c45a30a7ff789555d3722a4203670768faf81da2a40adfed3ec5cdeb5da06f04be81ddb53b9db7e
eea8114657 build: Enable unreachable-code-loop-increment (Jonathan Schoeller)
d15db4b1fc refactor: Fix unreachable code in init arg checks (Jonathan Schoeller)
Pull request description:
Closes: #19017
In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on.
```shell
init.cpp:969:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
^~~
1 warning generated.
```
aa8d76806c/src/init.cpp (L968-L972)
To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one.
ACKs for top commit:
laanwj:
Code review ACK eea8114657
hebasto:
re-ACK eea8114657, only suggested changes applied since the [previous](https://github.com/bitcoin/bitcoin/pull/19131#pullrequestreview-421772387) review.
Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583
bd7e530f01 This PR adds initial support for type hints checking in python scripts. (Kiminuo)
Pull request description:
This PR adds initial support for type hints checking in python scripts.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
[Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
**Notes:**
* [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful.
* We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
```python
_opcode_instances = [] # type: List[CScriptOp]
```
to
```python
_opcode_instances:List[CScriptOp] = []
```
for type hints that are **not** function parameters and function return types.
**Useful resources:**
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
ACKs for top commit:
fanquake:
ACK bd7e530f01 - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).
MarcoFalke:
ACK bd7e530f01 fine with me
Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
Migrates the CScriptNum decode tests into a unit test, and moved some
changes made in #14816. Made possible by the integration of
test_framework unit testing in #18576. Further extends the original
test with larger ints, similar to the scriptnum_tests.cpp file. Adds
test to blocktools.py testing fn create_coinbase() with CScriptNum
decode.
facede18a4 test: Check that invalid witness destinations can not be imported (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK facede18a4 -- thanks for adding!
Tree-SHA512: 87000606fac2e6f2780ca75cdeeb2dc1f0528d9b8f13e4156e8304ce7a6b1eb014781b6f0c59d11544bf360ba3dc5f99549470b0876132e189b9107f2c6bb38d
7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
fa7d3a8890 test: Add missing sync_blocks to wallet_hd (MarcoFalke)
eeeed51f58 test: pep-8 wallet_hd (MarcoFalke)
Pull request description:
This fixes the ` test_framework.authproxy.JSONRPCException: non-final (-26)` error when node 1 is one block behind of node 0. (height 122 vs 123)
ACKs for top commit:
promag:
Code review ACK fa7d3a8890.
Tree-SHA512: b549dce2e08c58b949168ed2013bfa176802c963d0d7e890f643c8792da5dade14d91441dfa74372f1f1d34d696e8900a2b60b4861c0ba2dce99f2a633ab27ff
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
Mypy is used in lint-python.sh to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
Useful resources:
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
Building with -Wunreachable-code-loop-increment causes a warning
due to always returning on the first iteration of the loop that
outputs errors on invalid args.
Collect all errors, and output them in a single error message
after the loop completes, resolving the warning and avoiding
popup hell by outputting a seperate message for each error.
facef3d413 doc: Explain that anyone can work on good first issues, move text to CONTRIBUTING.md (MarcoFalke)
fae2fb2a19 doc: Expand section on Getting Started (MarcoFalke)
100000d1b2 doc: Add headings to CONTRIBUTING.md (MarcoFalke)
fab893e0ca doc: Fix unrelated typos reported by codespell (MarcoFalke)
Pull request description:
Some random doc changes:
* Add sections to docs, so that they can be linked to
* Explain that anyone (even maintainers) are allowed to work on good first issues
* Expand section on Getting Started slightly
ACKs for top commit:
hebasto:
ACK facef3d413
fanquake:
ACK facef3d413
Tree-SHA512: 8998e273a76dbf4ca77e79374c14efe4dfcc5c6df6b7d801e1e1e436711dbe6f76b436f9cbc6cacb45a56827babdd6396f3bd376a9426ee7be3bb9b8a3b8e383
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
60ed33904c tests: implement base58_decode (10xcryptodev)
Pull request description:
implements TODO: def base58_decode
ACKs for top commit:
ryanofsky:
Code review ACK 60ed33904c. Just suggested changes since last review. Thank you for taking suggestions!
Tree-SHA512: b3c06b4df041a6d88033cd077a093813a688e42d0b9aa777c715e5fd69cfba7b1bf984428bd98417d3c15232d3d48bc9c163317564f9e1d562db6611c21e2c10
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
84ae0578b6 Add release notes about salvage changes (Andrew Chow)
ea337f2d03 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow)
9ea2d258b4 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow)
b426c7764d Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow)
2741774214 Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow)
ced95d0e43 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow)
07250b8dce walletdb: remove fAggressive from Salvage (Andrew Chow)
8ebcbc85c6 walletdb: don't automatically salvage when corruption is detected (Andrew Chow)
d321046f4b wallet: remove -salvagewallet (Andrew Chow)
cdd955e580 Add basic test for bitcoin-wallet salvage (Andrew Chow)
c87770915b wallettool: Add a salvage command (Andrew Chow)
Pull request description:
Removes the `-salvagewallet` startup option and adds a `salvage` command to the `bitcoin-wallet` tool. As such, `-salvagewallet` is removed. Additionally, the automatic salvage that is done if the wallet file fails to load is removed.
Lastly the salvage code entirely is moved out entirely into `bitcoin-wallet` from `walletdb.{cpp/h}` and `db.{cpp/h}`.
ACKs for top commit:
jonatack:
ACK 84ae0578b6 feedback taken, and compared to my previous review, the bitcoin-wallet salvage command now seems to run and it exits without raising. The new test passes at both 9454105 and 84ae057 so as a sanity check I'd agree there is room for improvement, if possible.
MarcoFalke:
re-ACK 84ae0578b6🏉
Empact:
Code Review ACK 84ae0578b6
ryanofsky:
Code review ACK 84ae0578b6. Lot of small changes since previous review: added verify step before salvage, added basic test in new commit, removed unused scanstate variable and warnings parameter, tweaked various comments and strings, moved fsuccess variable declaration
meshcollider:
Concept / light code review ACK 84ae0578b6
Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af
fa80b4788b test: Remove global wait_until from p2p_getdata (MarcoFalke)
999922baed test: Default mininode.wait_until timeout to 60s (MarcoFalke)
fab47375fe test: pep-8 p2p_getdata.py (MarcoFalke)
Pull request description:
Using the global wait_until makes it impossible to adjust the timeout based on the hardware the test is running on.
Fix that by using the mininode member function.
So for example, `./test/functional/p2p_getdata.py --timeout-factor=0.04` gives a timeout of 2.4 seconds.
ACKs for top commit:
laanwj:
ACK fa80b4788b
Tree-SHA512: ebb1b7860a64451de2b8ee9a0966faddb13b84af711f6744e8260d7c9bc0b382e8fb259897df5212190821e850ed30d4d5c2d7af45a97f207fd4511b06b6674a
5308c97cca [test] Add test for cfheaders (Jim Posen)
f6b58c1506 [net processing] Message handling for getcfheaders. (Jim Posen)
3bdc7c2d39 [doc] Add comment for m_headers_cache (John Newbery)
Pull request description:
Support `getcfheaders` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
jkczyz:
ACK 5308c97cca
MarcoFalke:
re-ACK 5308c97cca , only change is doc related 🗂
theStack:
ACK 5308c97cca🚀
Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
there were two calls to disconnect_nodes that were no-ops. fixed one & removed
the other & added assertions to confirm node has no connections when creating
the unbroadcast transaction.
- add () to function to actually disconnect from p2pconn
- extract max interval into a constant
- disconnect at the end of a subtest rather than start of next
5edad5ce5d test: add -getinfo multiwallet functional tests (Jon Atack)
903b6c117f rpc: drop unused JSONRPCProcessBatchReply size arg, refactor (Jon Atack)
afce85eb99 cli: use GetWalletBalances() functionality for -getinfo (Jon Atack)
9f01849a49 cli: create GetWalletBalances() to fetch multiwallet balances (Jon Atack)
743077544b cli: lift -rpcwallet logic up to CommandLineRPC() (Jon Atack)
29f2cbdeb7 cli: extract connection exception handler, -rpcwait logic (Jon Atack)
Pull request description:
This PR is a client-side version of #18453, per review feedback there and [review club discussions](https://bitcoincore.reviews/18453#meeting-log). It updates `bitcoin-cli -getinfo` on the client side to display wallet name and balance for the loaded wallets when more than one is loaded (e.g. you are in "multiwallet mode") and `-rpcwallet=` is not passed; otherwise, behavior is unchanged.
before
```json
$ bitcoin-cli -getinfo -regtest
{
"version": 199900,
"blocks": 15599,
"headers": 15599,
"verificationprogress": 1,
"timeoffset": 0,
"connections": 0,
"proxy": "",
"difficulty": 4.656542373906925e-10,
"chain": "regtest",
"balance": 0.00001000,
"relayfee": 0.00001000
}
```
after
```json
$ bitcoin-cli -getinfo -regtest
{
"version": 199900,
"blocks": 15599,
"headers": 15599,
"verificationprogress": 1,
"timeoffset": 0,
"connections": 0,
"proxy": "",
"difficulty": 4.656542373906925e-10,
"chain": "regtest",
"balances": {
"": 0.00001000,
"Encrypted": 0.00003500,
"day-to-day": 0.00000120,
"side project": 0.00000094
}
}
```
-----
`Review club` discussion about this PR is here: https://bitcoincore.reviews/18453
This PR can be manually tested by building, creating/loading/unloading several wallets with `bitcoin-cli createwallet/loadwallet/unloadwallet` and running `bitcoin-cli -getinfo` and `bitcoin-cli -rpcwallet=<wallet-name> -getinfo`.
`wallet_multiwallet.py --usecli` provides regression test coverage on this change, along with `interface_bitcoin_cli.py` where this PR adds test coverage.
Credit to Wladimir J. van der Laan for the idea in https://github.com/bitcoin/bitcoin/issues/17314 and https://github.com/bitcoin/bitcoin/pull/18453#issuecomment-605431806.
ACKs for top commit:
promag:
Tested ACK 5edad5ce5d.
jnewbery:
utACK 5edad5ce5d
meshcollider:
Code review ACK 5edad5ce5d
Tree-SHA512: 4ca36c5f6c49936b40afb605c44459c1d5b80b5bd84df634007ca276b3f6c102a0cb382f9d528370363ee32c94b0d7ffa15184578eaf8de74179e566c5c5cee5
fad798be76 test: Default --previous-releases to false if dir is empty (MarcoFalke)
faf1c3cc58 test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option (MarcoFalke)
Pull request description:
The "auto-detection" feature is kept in place, but making it an option allows to properly document it. For example, on my machine I get:
```
$ ./test/functional/wallet_disable.py --help | grep previous-releases
--previous-releases Force test of previous releases (default: False)
ACKs for top commit:
Sjors:
re-tACK fad798b
Tree-SHA512: a7377d0d5378be0a50be278d76396cc403583617b5fc43467773eee706df698acf3f4e67651491183b9b43a8e1816b052e4c17b90272b7ec4b6ac134ad811400
1ed52fbb4d Remove IBD check in sethdseed (Andrew Chow)
b1810a145a Test that keys from inactive seeds are generated (Andrew Chow)
c93082ece4 Generate new keys for inactive seeds after marking used (Andrew Chow)
45f2f6a0e8 Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow)
b59b4504ab have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow)
Pull request description:
Largely implements the suggestion from https://github.com/bitcoin/bitcoin/pull/17484#issuecomment-560845316.
After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially.
The indexes and internal-ness of a key is gotten by checking it's key origin data.
Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed.
A test case for this is added as well which fails on master.
ACKs for top commit:
ryanofsky:
Code review ACK 1ed52fbb4d. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this
ariard:
Code Review ACK 1ed52fb
jonatack:
ACK 1ed52fbb4d thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack.
Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f
amitiuttarwar:
ACK 651f1d816f🎉
MarcoFalke:
Review ACK 651f1d816f
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
0ea5d70b47 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8 Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)
Pull request description:
Related to: #18428
When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.
ACKs for top commit:
MarcoFalke:
ACK 0ea5d70b47
Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
4a614ff88a test: explicit imports from test_framework.messages in p2p_invalid_messages.py (Sebastian Falbesoner)
b35e1d2471 test: add inventory type constant MSG_CMPCT_BLOCK (Sebastian Falbesoner)
eeaaa58d2c test: replace inv type magic numbers by constants (Sebastian Falbesoner)
Pull request description:
Many functional tests still use magic numbers for inventory types, either passed to the `CInv` constructor or for comparing the `type` member of `CInv`. This PR replaces all of those by constants in the module `test_framework.messages` that have been introduced in commit c32cf9f622: `MSG_TX` (1) or `MSG_BLOCK` (2).
It also introduces a new constant `MSG_CMPCT_BLOCK` (naming as in `src/protocol.h`) and uses it to replace the remaining magic numbers.
The occurences of the magic numbers were identified through `grep`ing for `CInv(` and `type ==`. The idea was first to create a scripted-diff, but since also adding missing `import`s is needed, this would be non-trivial. Besides, also some unneeded comments like `# 2 == "Block"` could be removed.
ACKs for top commit:
gzhao408:
ACK [`4a614ff`](4a614ff88a)
Tree-SHA512: 4ba4fdef9f3eef7fd5ac72cb03ca3524863d1ae292161c550424a4c1047283fa2d2e7e03017d1fbae3652b3cb14f08b8d4b368403f3f209993aef3f2e2b22784
faf45d1f1f http: Avoid crash when g_thread_http was never started (MarcoFalke)
fa12a37b27 test: Replace inline-comments with logs, pep8 formatting (MarcoFalke)
fa83b39ff3 init: Remove confusing and redundant InitError (MarcoFalke)
Pull request description:
Avoid a crash during shutdown when the init sequence failed for some reason
ACKs for top commit:
promag:
Tested ACK faf45d1f1f.
ryanofsky:
Code review ACK faf45d1f1f. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test
hebasto:
ACK faf45d1f1f, tested on Linux Mint 19.3 with the following patch:
Tree-SHA512: 59632bf01c999e65c724e2728ac103250ccd8b0b16fac19d3a2a82639ab73e4f2efb86c78e63c588a5954625d8d0cf9545e2a7e070e6e15d2a54beeb50e00b61
- mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata),
so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete
('unbroadcast' = False) otherwise the state may change in between calls
- update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list
of txids to complete initial broadcast
- make mempool_packages.py wait because it compares entries using getrawmempool and
getmempoolentry
- expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast
- makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set
before and after originating node connects to peers (adds this in mempool_unbroadcast.py)
- adds mempool method IsUnbroadcastTx to query for tx inclusion in mempool's unbroadcast set
412d5fe879 QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr)
2abe8cc3b7 Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr)
Pull request description:
#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT.
Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation.
ACKs for top commit:
sipa:
ACK 412d5fe879
jnewbery:
Tested ACK 412d5fe879.
Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615
Modifies the existing --factor flag to --timeout-factor to better express intent.
Adds rules to disable timeout if --timeout-factor is set to 0.
Modfies --timeout-factor help doc to inform users about this feature.
8a22fd0114 avoided os-dependant path (Ferdinando M. Ametrano)
Pull request description:
The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust
ACKs for top commit:
MarcoFalke:
ACK 8a22fd0114
Tree-SHA512: 813f27aea33f97c8afac52e716a55fc5d7fb69621023aba99d40df7e1d145e0ec8d1eee49ddd403b219bf0e0e168e0e987b05c78eaef611f744d99bf2fc8bc91
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd and
7e89994133 from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
faa26d3744 test: Remove RPCOverloadWrapper boilerplate (MarcoFalke)
Pull request description:
There are too many wrappers in test_node already, so at least the code that implements the wrappers should be as minimal as possible.
ACKs for top commit:
laanwj:
code review ACK faa26d3744
Tree-SHA512: 94e593907de22187524e2445afb3101e40b3b599d4b4015aa8c6ca902d7586ff9daf520828759029d199a3af79e61b96b490a822a5a193ac7bf946beacb11a24
23083856a5 [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25a [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a5 the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a5
jkczyz:
re-ACK 23083856a5
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5🌳
theStack:
ACK 23083856a5
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
9847e205bf [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e0 [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c8 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
d135c29476 [ci] make list of previous releases to download a setting (Sjors Provoost)
9c246b873c [test] backwards compatibility: bump v0.19.0.1 to v0.19.1 (Sjors Provoost)
89a28e02fa [test] add v0.16.3 backwards compatibility test (Sjors Provoost)
Pull request description:
Thanks to #18774's `adjust_bitcoin_conf_for_pre_17` we can now test backwards compatibility for v0.16.3, both for sync and loading a recent wallet.
This PR bumps v0.19.0.1 to v0.19.1.
I also made the version list consistent for the `contrib/devtools/previous_release.sh` instruction, between both tests.
ACKs for top commit:
MarcoFalke:
ACK d135c29476
Tree-SHA512: 5ff137a7a934237fa220f1c2807ce9abeeb75929266558bf3e4045bec7dfcd0a8747fa74d700065c568330b18badf58c60c308eb13d1eed444d4bbfe6decc48b
fa2cce4391 wallet: Remove trailing whitespace from potential translation strings (MarcoFalke)
fa59cc1c97 wallet: Report full error message in wallettool (MarcoFalke)
fae7776690 wallet: Avoid translating RPC errors when creating txs (MarcoFalke)
fae51a5c6f wallet: Avoid translating RPC errors when loading wallets (MarcoFalke)
Pull request description:
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
`CreateWalletFromFile` does not know its caller, so this commit changes it
to return a `bilingual_str` to the caller.
Fixes#17072
ACKs for top commit:
laanwj:
ACK fa2cce4391, checked that no new translation messages are added compared to master.
hebasto:
ACK fa2cce4391
Tree-SHA512: c6a943ae9c3689ea3c48c20d26de6e4970de0257a1f1eec57a2bded67a4af9dcc5c45b2d64659d6fb4c4bc4d8103e28483ea3d14bb850df8db0ff9e8e5c77ee2
489ebfd7a1 tests: feature_backwards_compatibility.py test downgrade after upgrade (Andrew Chow)
Pull request description:
After upgrading the node, try to go back to the original version to make sure that using a newer node version does not prevent the wallet file from being downgraded again.
ACKs for top commit:
MarcoFalke:
ACK 489ebfd7a1
Tree-SHA512: 86231de6514b3657912fd9d6621212166fd2b29b591fc97120092c548babcf1d6f50b5bd103b28cecde395a26809134f01c1a198725596c3626420de3fd1f017
2742c34286 test: add factor option to adjust test timeouts (Harris)
Pull request description:
This PR adds a new option **factor** that can be used to adjust timeouts in various functional tests.
Several timeouts and functions from `authproxy`, `mininode`, `test_node` and `util` have been adapted to use this option. The factor-option definition is located in `test_framework.py`.
Fixes https://github.com/bitcoin/bitcoin/issues/18266
Also Fixes https://github.com/bitcoin/bitcoin/issues/18834
ACKs for top commit:
MarcoFalke:
Thanks! ACK 2742c34286
Tree-SHA512: 6d8421933ba2ac1b7db00b70cf2bc242d9842c48121c11aadc30b0985c4a174c86a127d6402d0cd73b993559d60d4f747872d21f9510cf4806e008349780d3ef
After upgrading the node, try to go back to the original version to make
sure that using a newver node version does not prevent the wallet file
from being downgraded again.
