f1b4503114 bumpfee test: exit loop at proper time with new fee value being compared (Gregory Sanders)
2e4edc68f9 Add some test logging to wallet_bumpfee.py (Gregory Sanders)
Pull request description:
In the loop we accidentally used `origfee` which is not the value to check, and also allowed the loop to exit too early since the new fee must be strictly greater than `0.0005`.
Also converted/added a bunch of logging from comments.
Resolves https://github.com/bitcoin/bitcoin/issues/17716
ACKs for top commit:
MarcoFalke:
ACK f1b4503114🏈
Tree-SHA512: eb73297fc82b09b9ec08d85ba3f0bec662119d0ff63ccf5d978a7bad6a674b5915f5ed021ec42f72a732c9ee7af43212d1de87361f50a970df7755caec96f6d8
ffff9dcdc3 test: Explain why test logging should be used (MarcoFalke)
Pull request description:
Background is that some tests don't have any `self.log` call at all. Thus there are no "anchor points" and those tests are hard to debug because the logs can't easily be parsed by a human.
ACKs for top commit:
jonatack:
ACK ffff9dcdc3
instagibbs:
ACK ffff9dcdc3
fanquake:
re-ACK ffff9dcdc3
Tree-SHA512: 08d962e85c4892c2a0c58feb5dc697c680a9d68e41a79417da6fcd415e0c5c735c4533a985cf225bb89deb5ca717d9bedf990657958079185804caa512b10f5a
1ba3e1cc21 init: move asmap code earlier in init process (Jon Atack)
5ba829e12e rpc: fix getpeerinfo RPCResult `mapped_as` type (Jon Atack)
c90b9a2399 net: extract conditional to bool CNetAddr::IsHeNet (Jon Atack)
819fb5549b logging: asmap logging and #include fixups (Jon Atack)
dcaf543ba0 test: add functional test for an empty, unparsable asmap (Jon Atack)
b8d0412b21 config: separate the asmap finding and parsing checks (Jon Atack)
81c38a2497 config: enable passing -asmap an absolute file path (Jon Atack)
fbe9b024f0 config: use default value in -asmap config (Jon Atack)
08b992675c test: add feature_asmap functional tests (Jon Atack)
Pull request description:
This PR builds on PR #16702 to add functional tests / sanity checks and user-facing refinements for passing `-asmap` to configure ASN-based IP bucketing in addrman. As per our review discussion in that PR, the idea here is to handle aspects like functional tests and config arg handling that can help the PR be merged while enabling the author to focus on the bucketing itself.
- [x] add feature functional tests to verify node behaviour and debug log output when launching
- `bitcoind` with no `-asmap` arg
- `bitcoind -asmap=RELATIVE_FILENAME` to the unit test data skeleton asmap
- `bitcoind -asmap` with no filename specified using the default asmap file
- `bitcoind -asmap` with no filename specified and a missing default asmap file
- [x] add the ability to pass absolute path filenames to the `-asmap` config arg in addition to datadir-relative path filenames as per https://github.com/bitcoin/bitcoin/pull/16702#discussion_r361300447, and add test coverage
- [x] separate the asmap file finding and parsing checks, which allows adding tests for the case of a found but unparseable or empty asmap
- [x] add test for an empty asmap
- [x] various asmap fixups
- [x] move the asmap init code earlier in the init process to provide immediate feedback when passing an `-asmap` config arg. This speeds up the `feature_asmap` functional test from 60 to 5 seconds! Credit to Wladimir J. van der Laan for the suggestion.
ACKs for top commit:
practicalswift:
ACK 1ba3e1cc21 -- diff looks correct
fanquake:
ACK 1ba3e1cc21
Tree-SHA512: e9094460a597ac5597449acfe631c87b71d3ede6a12c7ae61b26d1161b3eefed8e7e25c4fb0505864cebd89300b7c4cf9378060aa9155441029315df15fa3283
fa6df0de53 test: Bump timeouts to accomodate really slow disks (MarcoFalke)
Pull request description:
Needed these patches locally for some arm machines with slow storage
ACKs for top commit:
practicalswift:
ACK fa6df0de53
fanquake:
ACK fa6df0de53
Tree-SHA512: 22f2f6f7ed05f26013431126bb179b029dbc931f02d0e58f8970c6d477f43e3106d76c9732942034cb2cfcb827191e338a082f953ccb69531a19ee6dab9a7e1a
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
- move asmap #includes to sorted positions in addrman and init (move-only)
- remove redundant quotes in asmap InitError, update test
- remove full stops from asmap logging to be consistent with debug logging,
update tests
to verify node behaviour and debug log when launching bitcoind in these cases:
1. `bitcoind` with no -asmap arg, using /16 prefix for IP bucketing
2. `bitcoind -asmap=<relative path>`, using the unit test skeleton asmap
3. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
4. `bitcoind -asmap` with no file specified, and a missing default asmap file
The tests are order-independent. The slowest test (missing default asmap file)
is placed last.
1ef28b4f7c Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7c, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7c
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
this also fixes a bug that was uncovered with this checks:
for the BIP112 version 1 tx tests, certain txs (bip112txs_vary_OP_CSV_v1) have
been sent twice due to a typo, leading also to a failure as expected but for the
wrong reason
With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
-> stack is empty
-> top item on stack is negative (< 0)
-> top item on stack has disable flag unset and at least one of
four other conditions is true (contains the core CSV logic)
This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
fa45d60646 test: Reduce unneeded whitelist permissions in tests (MarcoFalke)
Pull request description:
It makes the tests confusing and fragile when overwriting default command line values that are not needed to be overwritten.
ACKs for top commit:
fanquake:
ACK fa45d60646
laanwj:
ACK fa45d60646
Tree-SHA512: 8ae5ad8c6be156b1a983adccbca8d868ef841e00605ea88e24227f1b7493987c50b3e62e68dd7dc785ad73d6e14279eb13d7a151cb0a976426fe2fd63ce5cbcd
To test the custom descendant limit on node1 (passed by the argument
-limitdescendantcount), we check for four conditions:
-> the # of txs in the node1 mempool is equal to the limit
(plus 1 for the parent tx, plus the # txs from the previous ancestor
test which are still in)
-> all txs in node1 mempool are a subset of txs in node0 mempool
-> part of the constructed descendant-chain (the first ones up to the
limit) are contained in node1 mempool
-> the remaining part of the constructed descendant-chain (all after the
first ones up to the limit) is *not* contained in node1 mempool
facb71576c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
This tests that a mempool transaction expires after a given timeout
and its children are removed as well.
Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
a user definable expiry timeout via the -mempoolexpiry=<n> command
line argument (<n> is the timeout in hours) are tested.
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
19a354b11f Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f
MarcoFalke:
ACK 19a354b11f
promag:
Code review ACK 19a354b11f.
meshcollider:
utACK 19a354b11f
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
900d8f6f70 util: Disallow network-qualified command line options (Russell Yanofsky)
Pull request description:
Previously these were allowed but ignored.
This change implements one of the settings simplifications listed in #17508. Change includes release notes.
ACKs for top commit:
laanwj:
ACK 900d8f6f70
Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad
1abcecc40c Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón)
Pull request description:
Simply avoiding the hardcoded string in more places for consistency.
It can also allow for more easily reusing tests for other chains other than regtest.
Separated from #8994 .
Continues #16509 .
It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO.
ACKs for top commit:
Sjors:
re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files.
elichai:
Code review ACK 1abcecc40c
ryanofsky:
Code review ACK 1abcecc40c.
ryanofsky:
Code review ACK 1abcecc40c
Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
b951b0973c on startup, write config options to debug.log (Larry Ruane)
Pull request description:
When a developer is examining `debug.log` after something goes wrong, it's often useful to know the exact options the failing instance of `bitcoind` was started with. Sometimes the `debug.log` file is all that's available for the analysis. This PR logs the `bitcoin.conf` entries and command-line arguments to `debug.log` on startup.
ACKs for top commit:
MarcoFalke:
ACK b951b0973c🐪
jonatack:
ACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of `str` debug log in the added unit test.
Tree-SHA512: bbca4fb3d49f99261758302bde0b8b67300ccc72e7380b01f1f66a146ae8a008a045df0ca5ca9664caff034d0ee38ea7ef38a50f38374525608c07ba52790358
aaaae4d0eb test: Add p2p test for forcerelay permission (MarcoFalke)
fa6b57bcaa test: Fix whitespace in p2p_permissions.py (MarcoFalke)
faf40810d7 test: Make msg_tx a witness tx (MarcoFalke)
Pull request description:
The commit `test: Make msg_tx a witness tx` is needed so that the python mininode does not strip the witness from transactions before sending them over p2p. The commit should also be done to keep symmetry with msg_block. See:
* tests: Make msg_block a witness block #15982
ACKs for top commit:
laanwj:
ACK aaaae4d0eb
Tree-SHA512: b4b546c88f7f0576cb512f0872bc6bef9d4df65783803f226986e56175937f418aa1ed906417ac909f27f1fd521d64629621fda83250fa925c46ef9513db0e4c
