fa1cd9e1dd test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)
Pull request description:
This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.
ACKs for top commit:
laanwj:
Code review ACK fa1cd9e1dd
hebasto:
ACK fa1cd9e1dd, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
e36f802fa4 lint: add C++ code linter (fanquake)
c4be50fea3 remove usage of boost::bind (fanquake)
Pull request description:
`boost::bind` usage was removed in #13743. However a new usage snuck in as
part of 2bc4c3eaf9 (#15225).
ACKs for top commit:
hebasto:
ACK e36f802fa4
practicalswift:
ACK e36f802fa4 -- patch looks correct
Tree-SHA512: 2b0387c5443c184bcbf7df4849db1ed1296ff82c7b4ff0aff18334a400e56a472a972d18234d3866531a088d7a8da64688e58dc9f15daaad4048697c759d55ce
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612
jnewbery:
utACK fb56d37612
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
36ec9801a4 test: Add chacha20 test vectors in muhash (Fabian Jahr)
0e2b400fea test: Add basic Python/C++ Muhash implementation parity unit test (Fabian Jahr)
b85543cb73 test: Add Python MuHash3072 implementation to test framework (Pieter Wuille)
ab30cece0e test: Move modinv to util and add unit test (Fabian Jahr)
Pull request description:
This is the second in a [series of pull requests](https://github.com/bitcoin/bitcoin/pull/18000) to implement an Index for UTXO set statistics.
This pull request adds a Python implementation of Muhash3072, a homomorphic hashing algorithm to be used for hashing the UTXO set. The Python implementation can then be used to compare behavior with the C++ version.
ACKs for top commit:
jnewbery:
utACK 36ec9801a
laanwj:
Code review ACK 36ec9801a4
Tree-SHA512: a3519c6e11031174f1ae71ecd8bcc7f3be42d7fc9c84c77f2fbea7cfc5ad54fcbe10b55116ad8d9a52ac5d675640eefed3bf260c58a02f2bf3bc0d8ec208baa6
3340dbadd3 Remove -zapwallettxes (Andrew Chow)
Pull request description:
It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.
Alternative to #19700
ACKs for top commit:
meshcollider:
utACK 3340dbadd3
fanquake:
ACK 3340dbadd3 - remaining manpage references will get cleaned up pre-release.
Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
7356292e1d Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983 Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600c Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)
Pull request description:
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion:
1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)
ACKs for top commit:
laanwj:
code review ACK 7356292e1d
promag:
Code review ACK 7356292e1d.
Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
fa3d9ce325 rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc20 rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7 rpc: Remove unused return type from appendCommand (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK fa3d9ce325
promag:
Code review ACK fa3d9ce325.
Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
6d1f51343c [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)
Pull request description:
When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.
Note that when creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC.
See #7518 for historical background.
ACKs for top commit:
meshcollider:
Code review ACK 6d1f51343c
fjahr:
Code review ACK 6d1f51343c
Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
ca185cf5a1 doc: Document differences in bitcoind and bitcoin-qt locale handling (practicalswift)
Pull request description:
Document differences in `bitcoind` and `bitcoin-qt` locale handling.
Since this seems to be the root cause to the locale dependency issues we've seen over the years I thought it was worth documenting :)
Note that 1.) `QLocale` (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as `std::to_string`) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization.
ACKs for top commit:
hebasto:
re-ACK ca185cf5a1
Tree-SHA512: e51c32f3072c506b0029a001d8b108125e1acb4f2b6a48a6be721ddadda9da0ae77a9b39ff33f9d9eebabe2244c1db09e8502e3e7012d7a5d40d98e96da0dc44
d841301010 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz)
1343c86c7c test: Update wait_until usage in tests not to use the one from utils (Seleme Topuz)
Pull request description:
Replace global (from [test_framework/util.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L228)) `wait_until()` usages with the ones provided by `BitcoinTestFramework` and `P2PInterface` classes.
The motivation behind this change is that the `util.wait_until()` expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. `BitcoinTestFramework` offers a `wait_until()` which has an understandable amount of default `timeout` and a shared `timeout_factor`. Moreover, on top of these, `mininode.wait_until()` also has a shared lock.
closes#19080
ACKs for top commit:
MarcoFalke:
ACK d841301010🦆
kallewoof:
utACK d841301010
Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88
fafc9d5af4 test: Fix intermittent issue in wallet_bumpfee (MarcoFalke)
fa347b2f25 test: Select at least the fee in wallet_bumpfee to avoid negative amounts (MarcoFalke)
Pull request description:
With a "dirty" mempool a transaction might fail to be accepted intermittently. For example,
* https://travis-ci.org/github/bitcoin-core/gui/jobs/719916499#L6773 Fails acceptance
* https://travis-ci.org/github/bitcoin-core/gui/jobs/719916499#L6954 Test fails
Fix the issue by clearing the mempool between subtests
ACKs for top commit:
promag:
Code review ACK fafc9d5af4.
Tree-SHA512: 23fb6decb6343d19eafddcbdb7da0551f6be11325d1c97c30e563944000aeb02bcc4b24904d204b132c093dc1acf28445fa1fd08bfe8d8b52ddd1de51c33eeb6
d5800da519 [test] Remove final references to mininode (John Newbery)
5e8df3312e test: resort imports (John Newbery)
85165d4332 scripted-diff: Rename mininode to p2p (John Newbery)
9e2897d020 scripted-diff: Rename mininode_lock to p2p_lock (John Newbery)
Pull request description:
New contributors are often confused by the terminology in the test framework, and what the difference between a _node_ and a _peer_ is. To summarize:
- a 'node' is a bitcoind instance. This is the thing whose behavior is being tested. Each bitcoind node is managed by a python `TestNode` object which is used to start/stop the node, manage the node's data directory, read state about the node (eg process status, log file), and interact with the node over different interfaces.
- one of the interfaces that we can use to interact with the node is the p2p interface. Each connection to a node using this interface is managed by a python `P2PInterface` or derived object (which is owned by the `TestNode` object). We can open zero, one or many p2p connections to each bitcoind node. The node sees these connections as 'peers'.
For historic reasons, the word 'mininode' has been used to refer to those p2p interface objects that we use to connect to the bitcoind node (the code was originally taken from the 'mini-node' branch of https://github.com/jgarzik/pynode/tree/mini-node). However that name has proved to be confusing for new contributors, so rename the remaining references.
ACKs for top commit:
amitiuttarwar:
ACK d5800da519
MarcoFalke:
ACK d5800da519🚞
Tree-SHA512: 2c46c2ac3c4278b6e3c647cfd8108428a41e80788fc4f0e386e5b0c47675bc687d94779496c09a3e5ea1319617295be10c422adeeff2d2bd68378e00e0eeb5de
5da96210fc doc: release note for getpeerinfo last_block/last_transaction (Jon Atack)
cfef5a2c98 test: rpc_net.py logging and test naming improvements (Jon Atack)
21c57bacda test: getpeerinfo last_block and last_transaction tests (Jon Atack)
8a560a7d57 rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack)
02fbe3ae0b net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack)
Pull request description:
This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`.
This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in https://github.com/bitcoin/bitcoin/pull/19643#issuecomment-671093420.
Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549:
```text
<jonatack> i was specifically trying to observe and figure out how to test https://github.com/bitcoin/bitcoin/issues/19500
<jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail
<jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
<jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
<sipa> jonatack: nope; i suspect just nobody ever added it
<jonatack> sipa: thanks. will propose.
```
The last commit is optional, but I think it would be good to have logging in `rpc_net.py`.
ACKs for top commit:
jnewbery:
Code review ACK 5da96210fc
theStack:
Code Review ACK 5da96210fc
darosior:
ACK 5da96210fc
Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70
15ae4a17c4 test/fuzz: add a seed corpus generation option to the test_runner (Antoine Poinsot)
Pull request description:
This adds a startup option to test/fuzz/test_runner.py which allows to generate seed corpus to the passed `seed_dir` instead of using them.
ACKs for top commit:
MarcoFalke:
ACK 15ae4a17c4
Tree-SHA512: f80ad58e48cc45272eace33dbf377848f31cbd6a25433786d50e9700f70185dff6513f71d885d0727ed57a2aa49163bfbdbc51a8091e99b4b1bae71e1504e6a5
5067c5acc3 [test] Add test for getblockheader verboseness (Torhte Butler)
Pull request description:
Improve test coverage by adding a test for getblockheader with verbose argument set to false.
ACKs for top commit:
theStack:
ACK 5067c5acc3
Tree-SHA512: e55593f1026a89dc7b796fa985b4cbcdb596e91d80d42dfb0660bda1692aaa35749ec29f9cd7032803f6225afb323f085df1ef6a9982de87be8e098f7253cdd5
124e1ee134 doc: Add release notes for getindexinfo RPC (Fabian Jahr)
c447b09458 test: Add tests for getindexinfo RPC (Fabian Jahr)
667bc7a7f7 rpc: Add getindexinfo RPC (Fabian Jahr)
Pull request description:
As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (#14053, #18000) that can give a quick overview without the user having to parse the logs.
Feature summary:
- Adds new RPC `listindices` (placed in Util section)
- That RPC only lists the actively running indices
- For each index it gives the name, whether it is synced and up to which block height it is synced
ACKs for top commit:
laanwj:
Re-ACK 124e1ee134
jonatack:
Code review re-ACK 124e1ee per `git range-diff a57af89 47a5372 124e1ee` no change since my last re-ACK, rebase only
Tree-SHA512: 3b7174c87951e6457fef099f530337803906baf32fb64261410b8def2c0917853d6a1bf3059cd590b1cc1523608f8916dafb327a431d27ecbf8d7454406b5b35
It's also clearer to have `no_version_disconnect_node` send a message
other than version or verack in order to reach the peer discouragement
threshold.
7f13dfb587 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf69 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587
achow101:
ACK 7f13dfb587
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
9e7894357e test: speedup p2p_feefilter.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
fe3f0cc44e test: use wait_until for invs matching in p2p_feefilter.py (Sebastian Falbesoner)
6d941923c3 test: add logging for p2p_feefilter.py (Sebastian Falbesoner)
Pull request description:
This PR gives some love to the functional test `p2p_feefilter.py` by introducing the following changes:
* add missing log messages for the `test_feefilter` subtest (the main one)
* deduplicate code by using the utility function `wait_until` (already using the [recently introduced version](12410b1feb)) instead of a manual condition checking loop with `time.sleep`
* improve naming of the function `matchAllInvs` (more expressive name, snake_case) and moving it from global namespace to the connection class `FeefilterConn`
* speeding up the test significantly by the good ol' method of activating immediate tx relay (as seen on e.g. https://github.com/bitcoin/bitcoin/pull/17121, https://github.com/bitcoin/bitcoin/pull/17124, https://github.com/bitcoin/bitcoin/pull/17340, https://github.com/bitcoin/bitcoin/pull/17362, ...):
```
master branch:
$ time ./p2p_feefilter.py
...
real 0m39.367s
user 0m1.227s
sys 0m0.571s
PR branch:
$ time ./p2p_feefilter.py
...
real 0m9.386s
user 0m1.120s
sys 0m0.577s
```
ACKs for top commit:
instagibbs:
code review ACK 9e7894357e
jonatack:
re-ACK 9e78943 per `git range-diff 3ab2582 ea74a3c 9e78943`
Tree-SHA512: fe21c1c5413df9165fea916b5d5f609d3ba33e7b5c3364b38eb824fcc55d9e6abddf27116cbc0b325913d451a73c44542040fb916aec9c46f805c6e12f6f10cf
fa330ec2fe test: Remove confusing and broken use of wait_until global (MarcoFalke)
fa6583c30b ci: Set increased --timeout-factor by default (MarcoFalke)
Pull request description:
Assuming that tests don't have a logic error or race, setting a high timeout should not cause any issues. The tests will still pass just as fast in the fastest case, but it allows for some buffer in case of slow disks or otherwise starved ci machines.
Fixes#19729
ACKs for top commit:
hebasto:
ACK fa330ec2fe, I have reviewed the code, and it looks OK, I agree it can be merged.
Tree-SHA512: 3da80ee008c7b08bab5fdaf7804d57c79d6fed49a7d37b9c54fc89756659fcb9981fd10afc4d07bd90d93c1699fd410a0110a3cd34d016873759d114ce3cd82d
