223588b1bbc63dc57098bbd0baa48635e0cc0b82 Add a --descriptors option to various tests (Andrew Chow)
869f7ab30aeb4d7fbd563c535b55467a8a0430cf tests: Add RPCOverloadWrapper which overloads some disabled RPCs (Andrew Chow)
cf060628590fab87d73f278e744d70ef2d5d81db Correctly check for default wallet (Andrew Chow)
886e0d75f5fea2421190aa4812777d89f68962cc Implement CWallet::IsSpentKey for non-LegacySPKMans (Andrew Chow)
3c19fdd2a2fd5394fcfa75b2ba84ab2277cbdabf Return error when no ScriptPubKeyMan is available for specified type (Andrew Chow)
388ba94231f2f10a0be751c562cdd4650510a90a Change wallet_encryption.py to use signmessage instead of dumpprivkey (Andrew Chow)
1346e14831489f9c8f53a08f9dfed61d55d53c6f Functional tests for descriptor wallets (Andrew Chow)
f193ea889ddb53d9a5c47647966681d525e38368 add importdescriptors RPC and tests for native descriptor wallets (Hugo Nguyen)
ce24a944940019185efebcc5d85eac458ed26016 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly (Andrew Chow)
1cb42b22b11c27e64462afc25a94b2fc50bfa113 Generate new descriptors when encrypting (Andrew Chow)
82ae02b1656819f4bd5023b8955447e1d4ea8692 Be able to create new wallets with DescriptorScriptPubKeyMans as backing (Andrew Chow)
b713baa75a62335ab9c0eed9ef76a95bfec30668 Implement GetMetadata in DescriptorScriptPubKeyMan (Andrew Chow)
8b9603bd0b443e2f7984eb72bf2e21cf02af0bcb Change GetMetadata to use unique_ptr<CKeyMetadata> (Andrew Chow)
72a9540df96ffdb94f039b9c14eaacdc7d961196 Implement FillPSBT in DescriptorScriptPubKeyMan (Andrew Chow)
84b4978c02102171775c77a45f6ec198930f0a88 Implement SignMessage for descriptor wallets (Andrew Chow)
bde7c9fa38775a81d53ac0484fa9c98076a0c7d1 Implement SignTransaction in DescriptorScriptPubKeyMan (Andrew Chow)
d50c8ddd4190f20bf0debd410348b73408ec3143 Implement GetSolvingProvider for DescriptorScriptPubKeyMan (Andrew Chow)
f1ca5feb4ad668a3e1ae543d0addd5f483f1a88f Implement GetKeypoolOldestTime and only display it if greater than 0 (Andrew Chow)
586b57a9a6b4b12a78f792785b63a5a1743bce0c Implement ReturnDestination in DescriptorScriptPubKeyMan (Andrew Chow)
f866957979c23cefd41efa9dae9e53b9177818dc Implement GetReservedDestination in DescriptorScriptPubKeyMan (Andrew Chow)
a775f7c7fd0b9094fcbeee6ba92206d5bbb19164 Implement Unlock and Encrypt in DescriptorScriptPubKeyMan (Andrew Chow)
bfdd0734869a22217c15858d7a76d0dacc2ebc86 Implement GetNewDestination for DescriptorScriptPubKeyMan (Andrew Chow)
58c7651821b0eeff0a99dc61d78d2e9e07986580 Implement TopUp in DescriptorScriptPubKeyMan (Andrew Chow)
e014886a342508f7c8d80323eee9a5f314eaf94c Implement SetupGeneration for DescriptorScriptPubKeyMan (Andrew Chow)
46dfb99768e7d03a3cf552812d5b41ceaebc06be Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file (Andrew Chow)
4cb9b69be031e1dc65d8964794781b347fd948f5 Implement several simple functions in DescriptorScriptPubKeyMan (Andrew Chow)
d1ec3e4f19487b4b100f80ad02eac063c571777d Add IsSingleType to Descriptors (Andrew Chow)
953feb3d2724f5398dd48990c4957a19313d2c8c Implement loading of keys for DescriptorScriptPubKeyMan (Andrew Chow)
2363e9fcaa41b68bf11153f591b95f2d41ff9a1a Load the descriptor cache from the wallet file (Andrew Chow)
46c46aebb7943e1e2e96755e94dc6c197920bf75 Implement GetID for DescriptorScriptPubKeyMan (Andrew Chow)
ec2f9e1178c8e38c0a5ca063fe81adac8f916348 Implement IsHDEnabled in DescriptorScriptPubKeyMan (Andrew Chow)
741122d4c1a62ced3e96d16d67f4eeb3a6522d99 Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan (Andrew Chow)
2db7ca765c8fb2c71dd6f7c4f29ad70e68ff1720 Implement IsMine for DescriptorScriptPubKeyMan (Andrew Chow)
db7177af8c159abbcc209f2caafcd45d54c181c5 Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet (Andrew Chow)
78f8a92910d34247fa5d04368338c598d9908267 Implement SetType in DescriptorScriptPubKeyMan (Andrew Chow)
834de0300cde57ca3f662fb7aa5b1bdaed68bc8f Store WalletDescriptor in DescriptorScriptPubKeyMan (Andrew Chow)
d8132669e10c1db9ae0c2ea0d3f822d7d2f01345 Add a lock cs_desc_man for DescriptorScriptPubKeyMan (Andrew Chow)
3194a7f88ac1a32997b390b4f188c4b6a4af04a5 Introduce WalletDescriptor class (Andrew Chow)
6b13cd3fa854dfaeb9e269bff3d67cacc0e5b5dc Create LegacyScriptPubKeyMan when not a descriptor wallet (Andrew Chow)
aeac157c9dc141546b45e06ba9c2e641ad86083f Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet (Andrew Chow)
96accc73f067c7c95946e9932645dd821ef67f63 Add WALLET_FLAG_DESCRIPTORS (Andrew Chow)
6b8119af53ee2fdb4c4b5b24b4e650c0dc3bd27c Introduce DescriptorScriptPubKeyMan as a dummy class (Andrew Chow)
06620302c713cae65ee8e4ff9302e4c88e2a1285 Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (Andrew Chow)
Pull request description:
Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using `createwallet`.
Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.
Descriptors can also be imported with a new `importdescriptors` RPC.
Native descriptor wallets use the `ScriptPubKeyMan` interface introduced in #16341 to add a `DescriptorScriptPubKeyMan`. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, `DescriptorScriptPubKeyMan` does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with `disable_private_keys` needs to be used for watchonly things.
A `--descriptor` option was added to some tests (`wallet_basic.py`, `wallet_encryption.py`, `wallet_keypool.py`, `wallet_keypool_topup.py`, and `wallet_labels.py`) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (`importprivkey`, `importpubkey`, `importaddress`, `importmulti`, `addmultisigaddress`, `dumpprivkey`, `dumpwallet`, `importwallet`, and `sethdseed`).
ACKs for top commit:
Sjors:
utACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82 (rebased, nits addressed)
jonatack:
Code review re-ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82.
fjahr:
re-ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82
instagibbs:
light re-ACK 223588b
meshcollider:
Code review ACK 223588b1bbc63dc57098bbd0baa48635e0cc0b82
Tree-SHA512: 59bc52aeddbb769ed5f420d5d240d8137847ac821b588eb616b34461253510c1717d6a70bab8765631738747336ae06f45ba39603ccd17f483843e5ed9a90986
Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it
Introduce DescriptorScriptPubKeyMan as a dummy class
Add WALLET_FLAG_DESCRIPTORS
Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet
Create LegacyScriptPubKeyMan when not a descriptor wallet
Introduce WalletDescriptor class
WalletDescriptor is a Descriptor with other wallet metadata
Add a lock cs_desc_man for DescriptorScriptPubKeyMan
Store WalletDescriptor in DescriptorScriptPubKeyMan
Implement SetType in DescriptorScriptPubKeyMan
Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet
Implement IsMine for DescriptorScriptPubKeyMan
Adds a set of scriptPubKeys that DescriptorScriptPubKeyMan tracks.
If the given script is in that set, it is considered ISMINE_SPENDABLE
Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan
Implement IsHDEnabled in DescriptorScriptPubKeyMan
Implement GetID for DescriptorScriptPubKeyMan
Load the descriptor cache from the wallet file
Implement loading of keys for DescriptorScriptPubKeyMan
Add IsSingleType to Descriptors
IsSingleType will return whether the descriptor will give one or multiple scriptPubKeys
Implement several simple functions in DescriptorScriptPubKeyMan
Implements a bunch of one liners: UpgradeKeyMetadata, IsFirstRun, HavePrivateKeys,
KeypoolCountExternalKeys, GetKeypoolSize, GetTimeFirstKey, CanGetAddresses,
RewriteDB
Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file
Implement SetupGeneration for DescriptorScriptPubKeyMan
Implement TopUp in DescriptorScriptPubKeyMan
Implement GetNewDestination for DescriptorScriptPubKeyMan
Implement Unlock and Encrypt in DescriptorScriptPubKeyMan
Implement GetReservedDestination in DescriptorScriptPubKeyMan
Implement ReturnDestination in DescriptorScriptPubKeyMan
Implement GetKeypoolOldestTime and only display it if greater than 0
Implement GetSolvingProvider for DescriptorScriptPubKeyMan
Internally, a GetSigningProvider function is introduced which allows for
some private keys to be optionally included. This can be called with a
script as the argument (i.e. a scriptPubKey from our wallet when we are
signing) or with a pubkey. In order to know what index to expand the
private keys for that pubkey, we need to also cache all of the pubkeys
involved when we expand the descriptor. So SetCache and TopUp are
updated to do this too.
Implement SignTransaction in DescriptorScriptPubKeyMan
Implement SignMessage for descriptor wallets
Implement FillPSBT in DescriptorScriptPubKeyMan
FillPSBT will add our own scripts to the PSBT if those inputs are ours.
If an input also lists pubkeys that we happen to know the private keys
for, we will sign those inputs too.
Change GetMetadata to use unique_ptr<CKeyMetadata>
Implement GetMetadata in DescriptorScriptPubKeyMan
Be able to create new wallets with DescriptorScriptPubKeyMans as backing
Generate new descriptors when encrypting
Add IsLegacy to CWallet so that the GUI knows whether to show watchonly
add importdescriptors RPC and tests for native descriptor wallets
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Functional tests for descriptor wallets
Change wallet_encryption.py to use signmessage instead of dumpprivkey
Return error when no ScriptPubKeyMan is available for specified type
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
Implement CWallet::IsSpentKey for non-LegacySPKMans
tests: Add RPCOverloadWrapper which overloads some disabled RPCs
RPCOverloadWrapper overloads some deprecated or disabled RPCs with
an implementation using other RPCs to avoid having a ton of code churn
around replacing those RPCs.
Add a --descriptors option to various tests
Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.
Some tests are modified to work with --descriptors and run with that
option in test_runer:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py <---- wallet_keypool_hd.py actually
* wallet_keypool_topup.py
* wallet_labels.py
* wallet_avoidreuse.py
2aac093a3d60e446b85eebdf170ea6bed77bec92 test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b1293742a49aa196cb210ff345a7339e6 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e4cdd8b06207abc1387c68d7bfc04c1 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 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠
LarryRuane:
ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
fa1cd9e1ddc6918c3d600d36eadea71eebb242b6 test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93b4f5976e81793a4a63aa03a2c8c686 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d1b7ee552c627f9829b4494b817ce28 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 fa1cd9e1ddc6918c3d600d36eadea71eebb242b6
hebasto:
ACK fa1cd9e1ddc6918c3d600d36eadea71eebb242b6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
ed5cd12869e0691a785199d2d977ce5879095180 test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py (Dhruv Mehta)
f6f082b9343522bc8005f23937ac6ecf56548c98 test: remove `CNodeNoVersionIdle` from p2p_leak.py (Dhruv Mehta)
45cf55ccac94689e48dd0648ed2401918a778024 test: remove `CNodeNoVersionMisbehavior` from p2p_leak.py (Dhruv Mehta)
Pull request description:
- Removes `CNodeNoVersionMisbehavior` per recommendation at https://github.com/bitcoin/bitcoin/pull/19657#issuecomment-669926458
- Removes `CNodeNoVersionIdle` because it is similarly unnecessary
- As someone new to the codebase, I found it easier to understand it if `no_version_disconnect_node` tries to overwhelm the peer with any message that is not version/verack.
- Per recommendation at https://github.com/bitcoin/bitcoin/pull/19727#pullrequestreview-468093555, made a clear distinction between nodes(bitcoind) and peers(mininode interface implementations)
ACKs for top commit:
jnewbery:
tested ACK ed5cd12869e0691a785199d2d977ce5879095180
amitiuttarwar:
utACK ed5cd12869
Tree-SHA512: 310a24c91fd837e7f65177edb55fe6142fb3559fae7867c5cdd9c9a23b1a02202b935ca9a82633fa7649f3de2fa221f6da906a7b5e499fc20f7254085033757d
fa4dfd215f62e88d668311701735c332c264fa2a test: Wait until is_connected in add_p2p_connection (MarcoFalke)
Pull request description:
Moving the wait_until from the individual test scripts to the test framework simplifies two tests
ACKs for top commit:
jnewbery:
Code review ACK fa4dfd215f62e88d668311701735c332c264fa2a
theStack:
ACK fa4dfd215f☕
Tree-SHA512: 36eda7eb323614a4c4f9215f1d7b40b9f9c4036d1c08eb701ea705f3e2986fdabd2fc558965a6aadabeed861034aeaeef3c00f968ca17ed7a27e42e506cda87d
faa9a74c9e99eb43ba0d27fa906767ee88011aeb test: Fail wait_until early if connection is lost (MarcoFalke)
Pull request description:
Calling `minonode.wait_until` needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all `wait_until`, unless opted out.
ACKs for top commit:
jnewbery:
Code review ACK faa9a74c9e99eb43ba0d27fa906767ee88011aeb.
Tree-SHA512: 4be850b96e23b87bc2ff42c028a5045d6f5cdbc9482ce6a6ba01cc5eb26710dab9e2ed547c363aac4bd5825151ee9996fb797261420b631bceeddbfa698d1dec
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece 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 fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
4aff7a48a4e0f1075306f181a276b8a74c857022 test: check importing wallets when blocks are pruned throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
437b608df2/src/wallet/rpc/backup.cpp (L513-L518)
ACKs for top commit:
andrewtoth:
ACK 4aff7a48a4e0f1075306f181a276b8a74c857022
Tree-SHA512: fbbf6056cb3759f726b8a5ff25fca51bf47e973e5d655ec164e2bec88e2dbd3b243677869d2cf33af268ea635ca0f2e9f737c4734077fc5a936ac3a24ad4b88b
fadddd13eef4428f5fa7237583d4be41a9335cd9 test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e3d4984b8edff1d762dd4cba205d982 test: Misc cleanup (MarcoFalke)
fa1668bf5084a190b26b022b9e625a7be3defa6e test: Run pep-8 (MarcoFalke)
facd97ae0f0d816107aa3bc9de321244200636a0 scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK fadddd13ee on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
39a9ec579f023ab262a1abd1f0c869be5b1f3f4d Unconditionally check for fRelay field in test framework (Troy Giorshev)
Pull request description:
picking up #20411 (rebased onto master)
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead, we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
ACKs for top commit:
jnewbery:
utACK 39a9ec579f023ab262a1abd1f0c869be5b1f3f4d
Tree-SHA512: 13a23f1180b7121ba41cb85baa38094b41f4607a7c88b3384775177cb116e76faf5514760624f98a4e8a830767407c46753a7e0285158c33e0c6ce395de8f15c
fada8b019af104a0df7659ede2618f594bb3e78a test: Add missing assignment in mempool_resurrect.py (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: f438d85cd14a91eabfc380d9ee120cc7a7f9103cf0cd1cf565f675f386f82d966901c0ad3f60b8c462642fbf0a3791dbbd774f9b07668d22b58eb575c8d702c1
11a32722f09f1d81f34bd09b26248ba99f2e7f07 test: run mempool_resurrect.py even with wallet disabled (Michael Dietz)
Pull request description:
Another functional test rewritten as proposed in #20078
**Request for help:**
`node.gettransaction(txid)` fails for transactions sent with `wallet.send_self_transfer`. Even though the `txid`s look correct, are added to the mempool correctly, and removed from the mempool when a block is mined - all as expected.
However, `node.gettransaction(txid)` throws the error:
```sh
Traceback (most recent call last):
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in run_test
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in <lambda>
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
```
Anyone know what's going wrong / can point me in the right direction if I'm making a mistake, or `MiniWallet` needs to be improved for this to work correctly?
ACKs for top commit:
MarcoFalke:
ACK 11a32722f09f1d81f34bd09b26248ba99f2e7f07
Tree-SHA512: 13d83a13ec23920db716e99b68670e61329d1cc73b12063d85bc1679ee6425a9951da4d2e392ca1f27760be7be049ccdc6f504e192ed5cd24ed0ba003b66fab3
6b56c1f4d0d5857d9d61a81dc96db1b603c368b5 test: remove last_{block,header}_equals() in p2p_fingerprint.py (Sebastian Falbesoner)
136d96b71f94bde2c7471ed852d447ec008e3a30 test: use wait_for_{block,header} helpers in p2p_fingerprint.py (Sebastian Falbesoner)
Pull request description:
This small PR takes use of the message receiving helper functions `wait_for_block()` and `wait_for_header()` (from module `test_framework.p2p`) in the test `p2p_fingerprint.py`. It also simplifies the checks for very old stale blocks/headers requests by getting rid of the functions `last_block_equals()` and `last_header_equals()` and rather only testing that not any blocks/headers message is received at all. Unneeded sending of requests are also removed and calls to time.sleep(...) substituted by ping syncs.
ACKs for top commit:
guggero:
ACK 6b56c1f4
Tree-SHA512: 9114db70f3804adad4ab658236762d4fa73fef91158c5756dd1af2d24196ea740451b0028667e0c4047f1f89fe1355031921d3dfb973acc1370052a4bc12c2ab
4f67336f1105b7c34a9e8cdafa603edc1d899fb9 test: verify best blockhash after invalidating an unknown block (brunoerg)
Pull request description:
Fixes#26051
Verify the best blockhash is the same after invalidating an unknown block, not the whole `getchaintip` response.
ACKs for top commit:
instagibbs:
ACK 4f67336f1105b7c34a9e8cdafa603edc1d899fb9
Tree-SHA512: 2d71743c1d3a317ef7b750f88437df71d1aed2728d9edac8b763a343406e168b97865ab25ec4c89caf09d002e076458376618cbd0845496375f7179633c88af9
4b1d5a10537ab48e3457606ba1cf2ae26a1cb2b2 test: invalidating an unknown block throws an error (brunoerg)
Pull request description:
While playing with `invalidateblock`, I unintentionally tried to invalidate an unknown block and it threw an error. Looking at the tests I just realized there is no test coverage for this case. This PR adds it.
Top commit has no ACKs.
Tree-SHA512: 25286ead809b3ad022e759127ef3134b271fbe76cb7b50ec2b0c7e2409da8d1b01dc5e80afe73e4564cc9c9c03487a1fe772aea3456988552d2f9c8fb34c730b
4edc6893825fd8c45c53c81c73a6a7801e1b458c doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner)
Pull request description:
As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by https://github.com/bitcoin/bitcoin/pull/25792#discussion_r941180819).
ACKs for top commit:
kouloumos:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
1440000bytes:
ACK 4edc689382
w0xlt:
ACK 4edc689382
fanquake:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
1821d92b66 test: add activate_ehf_by_name (Alessandro Rezzi)
de38dca242 feat(consensus): Generalize ehf activation (Alessandro Rezzi)
Pull request description:
## Issue being fixed or feature implemented
Try to sign/mine any ehf deployment and not only mn_rr. As asked in the review this decouples (and improves) commit [e24cb23](e24cb239f8) from PR #5799
## What was done?
See commit description
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: 7112a5214b473dea29a892563e6598eca089d2e17fdff8bda08c7777738f1054d2cb07e0a7dccf66214911fec64a6441e84100273ac2ed52abe1d33fb960e8cc
76a84c0a6cac3df711b1ed907a46c905cb85c485 test: speedup wallet_coinbase_category.py (furszy)
Pull request description:
No need to create a chain (200 extra blocks), nor use the cache, for it.
Top commit has no ACKs.
Tree-SHA512: beec64ba6c580d475e19700371ae155f4f3d74325879802da83d02f4153a752d5829b8a4ed77e0c893e79cbc26f66389eed90ac2e8b03a59f6c630ee9333355c
ceec6808d331fa082407a734cd5f3c2f1c7d11b3 test: `-whitebind` and `-bind` with `-listen=0` should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9122e95f0/src/init.cpp (L872-L875)
ACKs for top commit:
laanwj:
Code review ACK ceec6808d331fa082407a734cd5f3c2f1c7d11b3
Tree-SHA512: 03068abe7199b1235f029871ab87a3dd4943738c592ad62d82cdcd3e0201e627624960bd3ea1fc6fc1e7da4b8e215ba3393d1cb8130e1108049f764e51dc75c0
5a8c321444c10c7c89c4222c0b47c2d83a1a9ea4 test: check for `getblocktxn` request with out-of-bounds tx index (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `getblocktxn` message handler, in the case that any of the contained indices is out-of-bounds:
a05876619a/src/net_processing.cpp (L2180-L2183)
ACKs for top commit:
dunxen:
ACK 5a8c321
Tree-SHA512: 2743c2c6d8aed57b22f825aefd60ba3e670321b60625a42ea7248e7b0fc41c73e9a5945153567c02824ba3b5f0fce7f4125bffc974973fc608b6ffbe49e14b65
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
that's a result of:
contrib/devtools/copyright_header.py update ./
it is not scripted diff, because it works differentlly on my localhost and in CI:
CI doesn't want to use git commit date which is mocked to 30th Dec of 2023
ea54ba2f42f6d0b23570c665c2369f977bf55cf6 [test] Fix port collisions caused by p2p_getaddr_caching.py (dergoegge)
f9682e75ac184a62c7e29287882df34c25303033 [test_framework] Set PortSeed.n directly after initialising params (dergoegge)
Pull request description:
This PR fixes the issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/25096#discussion_r892558783), to avoid port collisions between nodes spun up by the test framework.
Top commit has no ACKs.
Tree-SHA512: ec9159f0af90db636f7889d664c24e1430cf2bcb3c02a9ab2dcfe531b2a4d18f6e3a0f8ba73071bdf2f7db518df9d5d86a9cd06695e67644d20fe4515fac32b7
fa76d8d4d71d844e217686881d4f630eac3a8e10 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721738f47053d915e4c65f925838ad5b4 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2796d00eabaec56d831f9e9b1a569cc.
ACKs for top commit:
fanquake:
utACK fa76d8d4d71d844e217686881d4f630eac3a8e10
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
## Issue being fixed or feature implemented
On my local kubuntu linters have way too much spam
## What was done?
See each commit
## How Has This Been Tested?
Run locally. Amount of warnings decreased from thousands to fewer
amount. Excluding typos, they are:
```
src/coinjoin/client.cpp:1420:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:1426:5: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/client.cpp:655:26: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:593:33: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/coinjoin/server.cpp:630:106: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/governance/governance.cpp:1057:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1068:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1079:13: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1086:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1094:9: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1099:5: warning: C-style pointer casting [cstyleCast]
src/governance/governance.cpp:1486:34: warning: Consider using std::copy_if algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/commitment.cpp:102:5: warning: Consider using std::all_of or std::none_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/instantsend.cpp:820:38: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.cpp:831:102: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/llmq/quorums.h:300:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:301:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:302:17: warning: C-style pointer casting [cstyleCast]
src/llmq/quorums.h:303:17: warning: C-style pointer casting [cstyleCast]
src/spork.cpp:119:58: warning: Consider using std::any_of algorithm instead of a raw loop. [useStlAlgorithm]
src/statsd_client.cpp:234:63: warning: C-style pointer casting [cstyleCast]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
^---- failure generated from test/lint/lint-cppcheck-dash.sh
Consider install flake8-cached for cached flake8 results.
test/functional/data/invalid_txs.py: error: Source file found twice under different module names: "invalid_txs" and "data.invalid_txs"
test/functional/data/invalid_txs.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
test/functional/data/invalid_txs.py: note: Common resolutions include: a) adding `__init__.py` somewhere, b) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)
^---- failure generated from test/lint/lint-python.s
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
fix failures like https://gitlab.com/dashpay/dash/-/jobs/6175160403
## What was done?
use `minimumAmount` option in `listunspent` rpc call to avoid picking
coins that are too small for asset lock txes
## How Has This Been Tested?
run `feature_asset_locks.py`
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
faa94cb1675d8bd511eb593176cd07aa59465225 test: Check that invalid peer traffic is accounted for (MarcoFalke)
fae243f0cb92b5648d07d0a5033e2f4de862ae99 test: Remove confusing cast to same type (int to int) (MarcoFalke)
Pull request description:
Couldn't find a test for this and it seems something we should test, so I wrote one.
ACKs for top commit:
vasild:
ACK faa94cb16
practicalswift:
ACK faa94cb1675d8bd511eb593176cd07aa59465225: patch looks correct
Tree-SHA512: efcdd35960045cdfbd14480e16e0d1d09e81eb01670ac541ac2b105e1a63818a157c95853270242234a224880873e79957832bf4231374d7fe81de30f35e3abf
## Issue being fixed or feature implemented
To prevent spending withdrawal before that's finalized, mined and
chainlocked, next things should be true:
- txes with spending of unlock and unlock themselves do not receive
islocks. That's true, because IS excluded for txes with no inputs.
- When the unlock is removed from mempool, so are the children.
These functionality has no tests, but that's crucial for consensus be
fine.
## What was done?
Implemented checks to be sure that IS is not send for Withdrawal txes.
Functional test for asset_locks.py are refactored a bit to make it
easier to read.
## How Has This Been Tested?
This PR is tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
9913419cc9db5f8ce7afa0c3774468c330136064 test: remove type: comments in favour of actual annotations (fanquake)
Pull request description:
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than `# type:` comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
ACKs for top commit:
MarcoFalke:
review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
jnewbery:
Code review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
Tree-SHA512: 63aba5eef6c1320578f66cf8a6d85ac9dbab9d30b0d21e6e966be8216e63606de12321320c2958c67933bf68d10f2e76e9c43928e5989614cea34dde4187aad8
bff7c66e67aa2f18ef70139338643656a54444fe Add documentation to contrib folder (Troy Giorshev)
381f77be858d7417209b6de0b7cd23cb7eb99261 Add Message Capture Test (Troy Giorshev)
e4f378a505922c0f544b4cfbfdb169e884e02be9 Add capture parser (Troy Giorshev)
4d1a582549bc982d55e24585b0ba06f92f21e9da Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97bec09dd5fcc043d8659d8ec5dfb87c2 Add CaptureMessage (Troy Giorshev)
dbf779d5deb04f55c6e8493ce4e12ed4628638f3 Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe only some minor changes: 👚
jnewbery:
utACK bff7c66e67aa2f18ef70139338643656a54444fe
theStack:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
## Issue being fixed or feature implemented
Should fix failures like
https://gitlab.com/dashpay/dash/-/jobs/6107697452
## What was done?
See inline comments
## How Has This Been Tested?
Run lots of `feature_governance.py` in parallel multiple times
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
## Issue being fixed or feature implemented
This pull request is a follow-up to
[some](https://github.com/dashpay/dash/pull/5834#discussion_r1470105685)
[feedback](https://github.com/dashpay/dash/pull/5834#discussion_r1467009815)
received on [dash#5834](https://github.com/dashpay/dash/pull/5834) as
the patterns highlighted were present in different parts of the codebase
and hence not corrected within the PR itself but addressed separately.
This is that separate PR 🙂 (with some additional cleanup of my own)
## What was done?
* This pull request will remain a draft until
[dash#5834](https://github.com/dashpay/dash/pull/5834) as it will
introduce more changes that will need to be corrected in this PR.
* Code introduced that is unique to Dash Core (CoinJoin, InstantSend,
etc.) has been excluded from un-Dashification as the purpose of it is to
reduce backport conflicts, which don't apply in those cases.
* `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture
have been excluded as the former originates from
[dash#3668](https://github.com/dashpay/dash/pull/3668) and the latter
from [dash#3667](https://github.com/dashpay/dash/pull/3667) and are
distinct enough to be unique to Dash Core.
* There are certain Dashifications and SegWit-removals that prove
frustrating as it would break compatibility with programs that rely on
the naming of certain keys
* `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and
`getmempoolentry` return `vsize` which is currently an alias of `size`.
I have been advised to retain `vsize` in lieu of potential future
developments. (this was originally remedied in
219a1d08973e7ccda6e778218b9a8218b4aae034 but has since been dropped)
* `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all
return a value with the key `satoshis`. This is frustrating to rename to
`duffs` for compatibility reasons.
* `decodepsbt` returns (if applicable) `non_witness_utxo` which is
frustrating to rename simply to `utxo` for the same reason.
* `analyzepsbt` returns (if applicable) `estimated_vsize` which
frustrating to rename to `estimated_size` for the same reason.
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
HD wallets are old-existsing feature, appeared in Dash years ago, but
enabling HD wallets is not trivial task that requires multiple steps and
command line/rpc calls.
Let's have them enabled by default.
## What was done?
- HD wallets are enabled by default. Currently behavior `dashd`,
`dash-qt` are similar to run with option `-usehd=1`
- the rpc `upgradewallet` do not let to upgrade from non-HD wallet to HD
wallet to don't encourage user use non-crypted wallets (postponed till
v21)
- the initialization of ScriptPubKey is updated to be sure that encypted
HD seed is never written on disk (if passphrase is provided)
- enabled and dashified a script `wallet_upgradewallet.py` which test
compatibility between different versions of wallet
## What is not done?
- wallet tool still does not support passhprase, HD seed can appear on
disk
- there's no dialog that show user a mnemonic phrase and encourage him
to make a paper backup
Before removing a command line 'usehd' (backport bitcoin#11250) need to
make at least one major release for fail-over option (if someone wish to
use non-HD wallets only).
## How Has This Been Tested?
Run unit and functional tests.
Enabled new functional test `wallet_upgradewallet.py` that has been
backported long time ago but waited this PR to be enabled.
## Breaking Changes
HD wallets are created by default.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Fix failures like https://gitlab.com/dashpay/dash/-/jobs/6120923632
## What was done?
Handle disconnects and reconnection of the revoked MN in the right
place.
## How Has This Been Tested?
Run multiple `feature_dip3_v19.py` in parallel a few times
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
56010f92564a94b0ca6c008c0e6f74a19fad4a2a test: hoist p2p values to test framework constants (Jon Atack)
75447f0893f9ad9bf83d182b301d139430d8de1c test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack)
57960192a5362ff1a7b996995332535f4c2a25c3 test: refactor test_large_inv() into 3 tests with common method (Jon Atack)
e2b21d8a597c536a8617408d43958bfe9f98a442 test: add p2p_invalid_messages logging (Jon Atack)
9fa494dc0969c61d5ef33708a08923cca19ce091 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 56010f92564a94b0ca6c008c0e6f74a19fad4a2a
MarcoFalke:
ACK 56010f9256 🎛
Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
## Issue being fixed or feature implemented
kudos to kwvg to report issue: he pointed out that functional tests
randomly fail lately.
Bisect pointed out an exact commit: bitcoin#20027 - mockable time
everywhere
## What was done?
Added call `mn.node.mockscheduler` as it is done in 20027 for other
functional tests
## How Has This Been Tested?
Run 20 times. Without this patch 20% failures; with this patch - zero
failures.
```
test/functional/test_runner.py -j20 feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py feature_llmq_simplepose.py
```
## Breaking Changes
N/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
fa2ecadd0d3283a89d27772dc0275e76277ae17e test: Fix intermittent rpc_net issue (MarcoFalke)
Pull request description:
The test fails because getpeerinfo and getnettotals are not synchronised, so a `wait_until` is needed for each RPC (separately).
Fixes https://cirrus-ci.com/task/4663366629195776?command=ci#L5034
ACKs for top commit:
jnewbery:
utACK fa2ecadd0d3283a89d27772dc0275e76277ae17e
Tree-SHA512: 5ea7128801aab8dbe3d9e6737545ff4ee770e4a9c5a2096ba2339a688424f1879ccba6bf8bcb219983acf86eb28af06fc629586613e7fe28aeffadd2c98633e8
778cd0d88d8d6dd22d7f0fb740f3ca3dbb2280a1 [tests] Remove getnettotals/getpeerinfo consistency test (John Newbery)
Pull request description:
We make no guarantees about consistency between RPC calls.
Alternative to 18784
ACKs for top commit:
MarcoFalke:
review ACK 778cd0d88d8d6dd22d7f0fb740f3ca3dbb2280a1
troygiorshev:
ACK 778cd0d88d8d6dd22d7f0fb740f3ca3dbb2280a1 after reading discussion on 18784, code review, ran test
Tree-SHA512: 438333a111cc93a09680cec47f13fbe03557d4803e5d826aec6f72e5afea62a088622645f0756e8fd2c9182c2a69ccca867d4d6fed2250364bee2b6c834adb1a
fa299ac27364bd7a59e6fb7e0c4ce476f2deec40 test: Speed up wallet_resendwallettransactions test with mockscheduler RPC (MarcoFalke)
Pull request description:
Also fixes#20143
ACKs for top commit:
guggero:
ACK fa299ac2
Tree-SHA512: 024ced4aa5f5c266e24fd0583d47b45b19c2a6ae25a06fabeacaa0ac996eec0c45f11cc34b2df17d01759b78ed31a991aa86978aafcc76cb0017382f601bf85a
8f0b64fb513e8c6cdd1f115856100a4ef5afe23e Better error messages for invalid addresses (Bezdrighin)
Pull request description:
This PR addresses #20809.
We add more detailed error messages in case an invalid address is provided inside the 'validateaddress' and 'getaddressinfo' RPC calls. This also covers the case when a user provides an address from a wrong network.
We also add a functional test to test the new error messages.
ACKs for top commit:
kristapsk:
ACK 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e
meshcollider:
Code review ACK 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e
Tree-SHA512: ca0f806ab573e96b79e98d9f8c810b81fa99c638d9b5e4d99dc18c8bd2568e6a802ec305fdfb2983574a97a19a46fd53b77645f8078fb77e9deb24ad2a22cf93
… best height
## Issue being fixed or feature implemented
Platform wants to know the height of the bestchainlock when they call
submitchainlock; sooo we change the API of submitchainlock to also
return the height
## What was done?
Adjust API and tests
## How Has This Been Tested?
New tests added for this behavior
## Breaking Changes
Not really any; I **guess** that return value could be considered
breaking change; but going from nothing -> something feels unlikely to
break anything although it in theory could.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
## Issue being fixed or feature implemented
Dashmate wanted a way to know if it is safe to restart the masternode.
This new RPC indicates the number of active DKG sessions, and the number
of blocks until next potential DKG.
## What was done?
Examples of responses:
`{'active_dkgs': 0, 'next_dkg': 22}`
## How Has This Been Tested?
`feature_llmq_rotation.py` was updated
## Breaking Changes
no
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
RPC `getassetunlockstatuses` is now accepting an extra optional
parameter `height`.
When a valid `height` is passed, then the RPC returns the status of
AssetUnlock indexes up to this specific block. (Requested by Platform
team)
## What was done?
Note that in order to avoid cases that can lead to deterministic result,
when `height` is passed, then the only `chainlocked` and `unknown`
outcomes are possible.
## How Has This Been Tested?
`feature_asset_locks.py` was updated.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
82dee87933ed0714976ff4eb9657acfc13c6de84 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 75122780e2c46505d977e24c5612dfa9442ab754 ("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 82dee87933ed0714976ff4eb9657acfc13c6de84
Tree-SHA512: 296b8a701f851d482ef6200c6cbf0cf0257a79a828ac6dbc39b05d8c2d839c6fdb9d3f5a084015295cfa3eac7c11faa2f2d52e619c11627b04c75150eead8330
BACKPORT NOTICE
fixup psbt. all missing changes belongs to src/wallet/scriptpubkeyman.h/cpp ----- they are related to descriptor wallet!
-------------------
931dd4760855e036c176a23ec2de367c460e4243 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d1b4dcc55c8826873f340ab4196af21 [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29d327d01aebb98b0504f317eb19c3dc [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa3aeaa69d8a3a716f902f450d5eaaec FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b73387600d175aff8bd5e6624dd51f86e7 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 931dd4760855e036c176a23ec2de367c460e4243
jb55:
ACK 931dd4760855e036c176a23ec2de367c460e4243
achow101:
ACK 931dd4760855e036c176a23ec2de367c460e4243
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
5da96210fc2fda9fbd79531f42f91262fd7a9257 doc: release note for getpeerinfo last_block/last_transaction (Jon Atack)
cfef5a2c98b9563392a4a258fedb8bdc869c9749 test: rpc_net.py logging and test naming improvements (Jon Atack)
21c57bacda766a4f56ee75a2872f5d0f94e3901e test: getpeerinfo last_block and last_transaction tests (Jon Atack)
8a560a7d57cbd9f473d6a3782893a0e2243c55bd rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack)
02fbe3ae0bd91cbab2828cb7aa46f6493c82f026 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 5da96210fc2fda9fbd79531f42f91262fd7a9257
theStack:
Code Review ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
darosior:
ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70
b6834e312a6a7bb395ec7266bc9469384639df96 Avoid 'timing mishap' warnings when mocking (Pieter Wuille)
ec3916f40a3fc644ecbbaaddef6258937c7fcfbc Use mockable time everywhere in net_processing (Pieter Wuille)
Pull request description:
The fact that net_processing uses a mix of mockable tand non-mockable time functions made it hard to write functional tests for #19988.
I'm opening this as a separate PR as I believe it's independently useful. In some ways this doesn't go quite as far as it could, as there are now several data structures that could be converted to `std::chrono` types as well now. I haven't done that here, but I'm happy to reconsider that.
ACKs for top commit:
MarcoFalke:
ACK b6834e312a 🌶
jnewbery:
utACK b6834e312a6a7bb395ec7266bc9469384639df96
naumenkogs:
utACK b6834e3
Tree-SHA512: 6528a167c57926ca12894e0c476826411baf5de2f7b01c2125b97e5f710e620f427bbb13f72bdfc3de59072e56a9c1447bce832f41c725e00e81fea019518f0e
0fcaf731997c4989b869e42d8990f742637799c2 test: use explicit p2p objects where available (Oliver Gugger)
Pull request description:
This is a follow-up patch to #19804 as suggested by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/19804#discussion_r494950062).
To make the intent of the tests easier to understand, we reference the
p2p connection objects by their explicit names instead of the p2ps array.
ACKs for top commit:
theStack:
ACK 0fcaf731997c4989b869e42d8990f742637799c2
Tree-SHA512: 37db22185077beeadfa7245bb768b87d6b7a2cfb906c3c859ab92ec3d657122db7301777f0838e13dfc748f37303850144fb7553e6cb6c66903e304d6e10e659
10d61505fe77880d6989115defa5e08417f3de2d [test] remove confusing p2p property (gzhao408)
549d30faf04612d9589c81edf9770c99e3221885 scripted-diff: replace p2p with p2ps[0] in p2p_invalid_tx (gzhao408)
7a0de46aeafb351cffa3410e1aae9809fd4698ad [doc] sample code for test framework p2p objects (gzhao408)
784f757994c1306bb6584b14c0c78617d6248432 [refactor] clarify tests by referencing p2p objects directly (gzhao408)
Pull request description:
The `TestNode` has a `p2p` property which is an alias for `p2ps[0]`.
I think this should be removed because it can be confusing and misleading (to both the test writer and reviewer), especially if a TestNode has multiple p2ps connected (which is the case for many tests).
Another example is when a test has multiple subtests that connect 1 p2p and use the `p2p` property to reference it. If the subtests don't completely clean up after themselves, the subtests may affect one another.
The best way to refer to a connected p2p is use the object returned by `add_p2p_connection` like this:
```py
p2p_conn = node.add_p2p_connection(P2PInterface())
```
A good example is [p2p_invalid_locator.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_locator.py), which cleans up after itself (waits in both `wait_for_disconnect` and in `disconnect_p2ps`) but wouldn't need so much complexity if it just referenced the connections directly.
If there is only one connected, it's not really that tedious to just use `node.p2ps[0]` instead of `node.p2p` (and it can always be aliased inside the test itself).
ACKs for top commit:
robot-dreams:
utACK 10d61505fe77880d6989115defa5e08417f3de2d
jnewbery:
utACK 10d61505fe77880d6989115defa5e08417f3de2d
guggero:
Concept ACK 10d61505.
Tree-SHA512: 5965548929794ec660dae03467640cb2156d7d826cefd26d3a126472cbc2494b855c1d26bbb7b412281fbdc92b9798b9765a85c27bc1a97f7798f27f64db6f13
21f24336019d438e225c7bd6653871119883a4ee test: run mempool_spend_coinbase.py even with wallet disabled (Michael Dietz)
Pull request description:
Run the mempool spend coinbase test even when the wallet was not compiled, as proposed in #20078.
ACKs for top commit:
laanwj:
ACK 21f24336019d438e225c7bd6653871119883a4ee
Tree-SHA512: 301582c04376371cfa8f1ebb2418a4341b42ddcd9ad4f48b58bcf888d867a97bdc409972856b67a8339ac5e60124aefee82a049b4f7fc6bca7a18d7e92e090be
3b064fcb9dd3df6c438a440f0fea86e9cf7b5f57 test: run mempool_expiry.py even with wallet disabled (Michael Dietz)
Pull request description:
Run the mempool expiry test even when the wallet was not compiled, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
ACKs for top commit:
MarcoFalke:
ACK 3b064fcb9dd3df6c438a440f0fea86e9cf7b5f57
Tree-SHA512: 5860dc021d02bc3752268ec1e859505bec87174953223b34b1af8a8e4ab66d645458fbf9571c0b816a9de891c3ff41314996e580869671fccd6972c093e78154
5b77f8098de537898151ab116d0e547fd6ff9466 test: add p2p_lock acquires in p2p_leak_tx.py (Sebastian Falbesoner)
cc8c6823b4a8b74922f78ce6ce527ced9325bd49 test: use MiniWallet for p2p_leak_tx.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_leak_tx.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078. It also adds missing p2p_lock acquires that need to be held while modifying internal p2p Interface state (in this case the `last_message` dictionary) to avoid data races.
ACKs for top commit:
laanwj:
Code review ACK 5b77f8098de537898151ab116d0e547fd6ff9466
Tree-SHA512: 6661bc6e3491a9af4bf040f379e5955c525136397e99d3eadde92e247580d0d87efff750e6d3b1f6d9a4e578144a433a982f574ef056b44dd6bca33873a1bae6
e15344889aac50aadee9211ac34e466867d5862b Clarify blocksonly whitelistforcerelay test (t-bast)
Pull request description:
As discussed in https://github.com/bitcoin/bitcoin/issues/19943, this test may be a bit misleading to newcomers.
We underscore the fact that our peer needs to run a modified version of Bitcoin Core to actually relay transactions to a `blocksonly` node and benefit from the `whitelistforcerelay` parameter.
ACKs for top commit:
naumenkogs:
ACK e15344889aac50aadee9211ac34e466867d5862b
Tree-SHA512: cc3526ac26c40a2d878b0ad863008663040683fd21092fcdc93866c2b0a79db8c2d29767d1f70bf56195092fca2aa2961cdbee52b5f0b1ae757cece9cd206301
b128b566725a5037fdaea99940d1b9de5553d198 test: add logging for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
8ee3536b2b77aeb3a48df5b34effbc7345ef34d8 test: remove unused helpers random_transaction(), make_change() and gather_inputs() (Sebastian Falbesoner)
fddce7e199308d96e366d700dca982ef088ba98b test: use MiniWallet for mining_getblocktemplate_longpoll.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_getblocktemplate_longpoll.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078. Also adds missing log messages for the subtests.
This was the only functional test that used the `random_transaction` helper in `test_framework/util.py`, hence it is removed, together with other helpers (`make_change` and `gather_inputs`) that were again only used by `random_transaction`.
ACKs for top commit:
MarcoFalke:
ACK b128b566725a5037fdaea99940d1b9de5553d198
Tree-SHA512: 09a5fa7b0f5976a47040f7027236d7ec0426d5a4829a082221c4b5fae294470230e89ae3df0bca0eea26833162c03980517f5cc88761ad251c3df4c4a49bca46
faabc26a61873b2cd0390a21df571fe53c893c11 test: Replace getmempoolentry with testmempoolaccept in MiniWallet (MarcoFalke)
Pull request description:
This is a refactor to not use the return value of `sendrawtransaction` and `getmempoolentry` with the goal that submitting the tx to the mempool will become optional.
ACKs for top commit:
mjdietzx:
ACK faabc26a61873b2cd0390a21df571fe53c893c11
Tree-SHA512: 4260a59e65fed1c807530dad23f1996ba6e881bcce91995f5b498a0be6001f67b3e0251507c8801750fe105410147c68bb2f393ebe678c6a3a4d045e5d72fc19
a7599c80ebb9579df45e2d6ccf3168302cf42f03 test: run mempool_compatibility.py even with wallet disabled (Michael Dietz)
Pull request description:
Another functional test rewritten as proposed in https://github.com/bitcoin/bitcoin/issues/20078
ACKs for top commit:
MarcoFalke:
review ACK a7599c80ebb9579df45e2d6ccf3168302cf42f03 didn't test
Tree-SHA512: cc7a617e5489ed27bbdbdee85a82fa08525375061f7f4524577a6b8ecb340396adac88419b51f513be22ca53edd0a3bd5d572d9f43ffc2c18550b0ef9069d238
faf251d854e3a670533ea3e9087e82c92f3ae533 test: gettxoutproof duplicate txid (João Barbosa)
faf5eb45c4a08fbfd9a8c12534bca8adfe756ef2 test: Test empty array in gettxoutproof (MarcoFalke)
fa56e866e8ac08b35e775a4e37a4e5849e093c7d test: Run rpc_txoutproof.py even with wallet disabled (MarcoFalke)
faba790bd40b5a9e8849997785020790ff60571b test: MiniWallet: Default fee_rate in send_self_transfer, Pass in utxo_to_spend (MarcoFalke)
fa65a11d0c9a34ff7f4cc4efd53367794e751749 test: bugfix: Actually pick largest utxo (MarcoFalke)
Pull request description:
Run the consensus test even when the wallet was not compiled. Also:
* Minor bugfix in MiniWallet
* Two new test cases (one cherry-picked from #19847)
ACKs for top commit:
jnewbery:
utACK faf251d854e3a670533ea3e9087e82c92f3ae533. Thanks Marco!
kristapsk:
ACK faf251d854e3a670533ea3e9087e82c92f3ae533
Tree-SHA512: a5ab33695c88cfb3c369021d4506069c08ce298e24e891db55159130693ed3817444c72f6aad3f472235aa4597b2c601010af714411c2ec8ad9c2d2e0b00ecbc
fa188c9c59b8c3e43c31be01797f073e27a7bc10 test: Use MiniWalet in p2p_feefilter (MarcoFalke)
fa39c62eb7f39e7d249b8d46c075c4e7a9aec684 test: inline hashToHex (MarcoFalke)
Pull request description:
This introduces a minimalistic test wallet, which can be used as a drop in replacement for the Bitcoin Core wallet to create dummy transactions with a given fee rate.
ACKs for top commit:
jnewbery:
utACK fa188c9c59b8c3e43c31be01797f073e27a7bc10
Tree-SHA512: 0aad9cb14eea4f0055bd6a47cc8c8f82a16941b152598c3bf1e083aae84cca4ffa23f0b854a362a68be1b917deba1b5ec7c0207b63b0805d747ba9a7d1d82efe
84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
46004790588c24174a0bec49b540d158ce163ffd psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07d601fe6a67ad665fbc7591fe73c7de psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da198764d4648a10a61c485e7ab65e9e 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 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 (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 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3. 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 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
7984c39be11ca04460883365e1ae2a496aaa6c0e test framework: serialize/deserialize inv type as unsigned int (Jon Atack)
407175e0c2bc797599ebd9c0a1f2ec89ad7af136 p2p: change CInv::type from int to uint32_t (Jon Atack)
Pull request description:
Fixes UBSan implicit-integer-sign-change issue per https://github.com/bitcoin/bitcoin/pull/19610#issuecomment-680686460.
Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in https://github.com/bitcoin/bitcoin/pull/19590#pullrequestreview-455788826.
Closes#19678.
ACKs for top commit:
laanwj:
ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e
MarcoFalke:
ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e 🌻
vasild:
ACK 7984c39be
Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5
6d1f51343cf11b07cd401fbd0c5bc3603e185a0e [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 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e
fjahr:
Code review ACK 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e
Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
fa330ec2fe5f5ba68a8d43fff0b19584c0b1ff39 test: Remove confusing and broken use of wait_until global (MarcoFalke)
fa6583c30bf7d82cf7ffdae995f8f16524ad2c0d 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 fa330ec2fe5f5ba68a8d43fff0b19584c0b1ff39, I have reviewed the code, and it looks OK, I agree it can be merged.
Tree-SHA512: 3da80ee008c7b08bab5fdaf7804d57c79d6fed49a7d37b9c54fc89756659fcb9981fd10afc4d07bd90d93c1699fd410a0110a3cd34d016873759d114ce3cd82d
This backport is marked as full, not partial, but it has only refactorings
and non-witness related changes.
Included commits are:
- test: Update test framework p2p protocol version to 70016
- Rename AddInventoryKnown() to AddKnownTx()
- Add support for tx-relay via wtxid
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.
- Just pass a hash to AddInventoryKnown
Since it's only used for transactions, there's no need to pass in an inv type.
- Add wtxid to mempool unbroadcast tracking
## Issue being fixed or feature implemented
For platform needs `getrawtransactionmulti` will help to reduce amount
of rpc calls for sake of performance improvement.
## What was done?
Implemented new RPC, basic functional test, release note.
## How Has This Been Tested?
On testnet:
```
> getrawtransactionmulti '{"000000abbe61a4d9b9356cb1d7deb1132d0b444a62869e71c2f3aa8ce2361359":["6e3ef19a3f955ac75a1f84dae60d42bbe11548ef54e37033ff2d91b3c4a09e9c", "415d5fafd5ee24ada8b99c36df339785a3066170c0dca6bb1aa6a5b96cf51e35"], "0":["ec7090f01c0e9b6e29d3be8810b12c780d2fb34372a53b231ce18bb7d2f1e8b0"]}'
> getrawtransactionmulti '{"000000abbe61a4d9b9356cb1d7deb1132d0b444a62869e71c2f3aa8ce2361359":["6e3ef19a3f955ac75a1f84dae60d42bbe11548ef54e37033ff2d91b3c4a09e9c", "415d5fafd5ee24ada8b99c36df339785a3066170c0dca6bb1aa6a5b96cf51e35"], "0":["ec7090f01c0e9b6e29d3be8810b12c780d2fb34372a53b231ce18bb7d2f1e8b0"]}' true
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: pasta <pasta@dashboost.org>
d841301010914203fb5ef02627c76fad99cb11f1 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz)
1343c86c7cc1fc896696b3ed87c12039e4ef3a0c 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 d841301010914203fb5ef02627c76fad99cb11f1 🦆
kallewoof:
utACK d841301010914203fb5ef02627c76fad99cb11f1
Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88
12410b1feb80189061eb4a2b43421e53cbb758a8 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
d5800da5199527a366024bc80cad7fcca17d5c4a [test] Remove final references to mininode (John Newbery)
5e8df3312e47a73e747ee892face55ed9ababeea test: resort imports (John Newbery)
85165d4332b0f72d30e0c584b476249b542338e6 scripted-diff: Rename mininode to p2p (John Newbery)
9e2897d020b114a10c860f90c5405be029afddba 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 d5800da5199527a366024bc80cad7fcca17d5c4a 🚞
Tree-SHA512: 2c46c2ac3c4278b6e3c647cfd8108428a41e80788fc4f0e386e5b0c47675bc687d94779496c09a3e5ea1319617295be10c422adeeff2d2bd68378e00e0eeb5de
dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 refactor: test: use _ variable for unused loop counters (Sebastian Falbesoner)
Pull request description:
This tiny PR substitutes Python loops in the form of `for x in range(N): ...` by `for _ in range(N): ...` where applicable. The idea is indicating to the reader that a block (or statement, in list comprehensions) is just repeated N times, and that the loop counter is not used in the body, hence using the throwaway variable. This is already done quite often in the current tests (see e.g. `$ git grep "for _ in range("`). Another alternative would be using `itertools.repeat` (according to Python core developer Raymond Hettinger it's [even faster](https://twitter.com/raymondh/status/1144527183341375488)), but that doesn't seem to be widespread in use and I'm not sure about a readability increase.
The only drawback I see is that whenever one wants to debug loop iterations, one would need to introduce a loop variable again. Reviewing this is basically a no-brainer, since tests would fail immediately if a a substitution has taken place on a loop where the variable is used.
Instances to replace were found by `$ git grep "for.*in range("` and manually checked.
ACKs for top commit:
darosior:
ACK dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64
instagibbs:
manual inspection ACK dac7a111bd
practicalswift:
ACK dac7a111bdd3b0233d94cf68dae7a8bfc6ac9c64 -- the updated code is easier to reason about since the throwaway nature of a variable is expressed explicitly (using the Pythonic `_` idiom) instead of implicitly. Explicit is better than implicit was we all know by now :)
Tree-SHA512: 5f43ded9ce14e5e00b3876ec445b90acda1842f813149ae7bafa93f3ac3d510bb778e2c701187fd2c73585e6b87797bb2d2987139bd1a9ba7d58775a59392406
292828cd7744ec7eadede4ad54aa2117087c5435 [test] Test addr cache for multiple onion binds (dergoegge)
3382905befd23364989d941038bf7b1530fea0dc [net] Seed addr cache randomizer with port from binding address (dergoegge)
f10e80b6e4fbc151abbf1c20fbdcc3581d3688f0 [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge)
Pull request description:
The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): a8098f2cef/src/net.cpp (L2800-L2804)
For inbound onion connections `CNode::addr.GetNetwork()` returns `NET_UNROUTABLE` and `CNode::addrBind` is set to `127.0.0.1:<onion bind port>`. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.
To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using `CNode::ConnectedThroughNetwork()`) as well as the port of `CNode::addrBind`.
ACKs for top commit:
sipa:
utACK 292828cd7744ec7eadede4ad54aa2117087c5435
mzumsande:
Code Review ACK 292828cd7744ec7eadede4ad54aa2117087c5435
naumenkogs:
utACK 292828cd7744ec7eadede4ad54aa2117087c5435
Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f
ebfc308ea4b8851118e8194d837556bf443c329c test: add coverage for non-hex value to -minimumchainwork (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9ef5a10e2/src/init.cpp (L917-L919)
Passing a non-hex value to -minimumchainwork should throw an initial error.
ACKs for top commit:
laanwj:
Code review ACK ebfc308ea4b8851118e8194d837556bf443c329c
kristapsk:
re-ACK ebfc308ea4b8851118e8194d837556bf443c329c
Tree-SHA512: c665903757ae3b8b2480df97bb888e60ba4387b009fcb8031041822e87a155a0e4950ebe79873c1034f0826504521d82b1fdfdb5e8378b227d14ca545b8d4e11
6d7e46ce23217da53ff52f535879c393c02fa2b2 Remove `Warning:` (Prayank)
Pull request description:
Reason: I noticed that `Warning` is printed 2 times in `-getinfo` while reviewing https://github.com/bitcoin/bitcoin/pull/21832#issuecomment-851004943
Same string is used for GUI, log and stderr. If we need to add `Warning:` in GUI or other place we can always prepend to this string.
CLI:
```
Warnings: Unknown new rules activated (versionbit 28)
```
GUI:
ACKs for top commit:
MarcoFalke:
review ACK 6d7e46ce23217da53ff52f535879c393c02fa2b2
Tree-SHA512: 25760cf6d850f6c2216d651fa46574d2d21a9d58f4577ecb58f9566ea8cd07bfcd6679bb8a1f53575e441b02d295306f492c0c99a48c909e60e5d977ec1861f6
c133cdcdc3397a734d57e05494682bf9bf6f4c15 Cap listsinceblock target_confirmations param (Adam Stein)
Pull request description:
This addresses an issue brought up in #19587.
Currently, the `target_confirmations` parameter to `listsinceblock` is not checked for being too large. When `target_confirmations` is greater than one more than the current number of blocks, `listsinceblock` fails with error code -1. In comparison, when `target_confirmations` is less than 1, a -8 "Invalid parameter" error code is thrown.
This PR fixes the issue by returning a -8 "Invalid parameter" error if the `target_confirmations` value corresponds to a block with more confirmations than the genesis block. This happens if `target_confirmations` exceeds one more than the number of blocks.
ACKs for top commit:
laanwj:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15
ryanofsky:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15. Just suggested changes since last review. Thanks!
Tree-SHA512: 02680f4cb937d2c24d5019abd0ebfa188b8a50679a1e64e9c26bfe5c17eef6aea906832e6e2d492ba8a2ea160041bf185d66795ee691e340f6793db03c21b89a
