2578e6fc0f4af35f389cd8ff59825c874e0b72ac test: Fix race condition in IPC interface block propagation test (Fabian Jahr)
Pull request description:
CI failed on this condition here: https://github.com/bitcoin/bitcoin/actions/runs/19395398994/job/55494696022?pr=33878#step:9:3983
The check was added not too long ago in https://github.com/bitcoin/bitcoin/pull/33745 and the fix here switches the check to the node which actually produces the block. There are also some comments added to make the checks easier so understand.
Closes#33884
ACKs for top commit:
Sjors:
re-utACK 2578e6fc0f4af35f389cd8ff59825c874e0b72ac
maflcko:
lgtm ACK 2578e6fc0f4af35f389cd8ff59825c874e0b72ac
Tree-SHA512: bfb7ae44aede50a00d4096e1a9922f9b8df31ce4242e12863e329d0d1e714d8cb46c852f694c32314e4bd26b524535e3a6967b7c57861a9b00cf09831a950b99
28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2 test: check listdescriptors do not return a mix of hardened derivation marker (pythcoiner)
975783cb79e929260873c1055d4b415cd33bb6b9 descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() (pythcoiner)
Pull request description:
In `MiniscriptDescriptor::ToStringHelper()` only the `StringType::Private` variant of the `type` argument was handled. This PR implements serializing w/ all variants of `StringType` & add a functional test for the descriptor triggering the related issue.
Closes#31694: previously when calling `listdescriptors` RPC on a wallet containing a taproot descriptor w/ a (miniscript) taptree, origins of internal key & taptree were serialized w/ differents hardened derivation markers:
- origin of the internal key were serialized w/ `StringType::Normalized` type (using `h` as marker)
- origins of taptree keys were serialized w/ `StringType::Private` type (using `'` as marker)
Note: Origins in segwit (`wsh()`) miniscript descriptors were also serialized w/ `StringType::Private` type (`'` marker) and are now serialized w/ `StringType::Normalized` type (`h` marker).
ACKs for top commit:
sipa:
Code review ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
achow101:
ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
rkrux:
Concept ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
Tree-SHA512: 15d14000b5951ca69a64a05b9a0b138c48a07b81eaf2fa86b91ac20cc8735533355a787363c64ba88403dd8a56ef5232cba57d34bea80835a0f40774d62fbc2b
f53dbbc5057b6f676db4be9bc720898149f293fc test: Add functional tests for named argument parsing (zaidmstrr)
694f04e2bd34f994d81e27b68e4d7466a9a319f8 rpc: Handle -named argument parsing where '=' character is used (zaidmstrr)
Pull request description:
Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).
The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables `-named` by default in the `bitcoin rpc` interface; `bitcoin rpc` corresponds to `bitcoin-cli -named` as it's just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain "=" character. This splits the parameter into two parts first, before the "=" character, which treats this as the parameter name, but the other half is mostly passed as an empty string. Here, the first part of the string is an unknown parameter name; thus, an error is thrown. These types of errors are only applicable to those RPCs which might contain the `=` character as a parameter. Some examples are `finalizepsbt`, `decodepsbt`, `verifymessage` etc.
This is the one example of the error in `finalizepsbt` RPC:
```
./bitcoin-cli -named -regtest finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
error code: -8
error message:
Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA
```
This PR fixes this by updating the `vRPCConvertParams` table that identifies parameters that need special handling in `-named` parameter mode. The parser now recognises these parameters and handles strings with "=" char correctly, preventing them from being incorrectly split as parameter assignments.
ACKs for top commit:
ryanofsky:
Code review ACK f53dbbc5057b6f676db4be9bc720898149f293fc. Just applied comment & test suggestions since last review
kannapoix:
Code review ACK: f53dbbc5057b6f676db4be9bc720898149f293fc
achow101:
ACK f53dbbc5057b6f676db4be9bc720898149f293fc
Tree-SHA512: 1b517144efeff45a4c4256c27a39ddf187f1d6189d133402a45171678214a10ff2925c31edcfd556d67f85bd26d42f63c528b941b68c9880eab443f2c883e681
b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5 scripted-diff: fix leftover references to `policy/fees.h` (ismaelsadeeq)
Pull request description:
Fixes#33863
ryanofsky wrote
> I still see some references to the src/policy/fees.h file removed by this PR:
```
$ git grep -n policy/fees.h
src/wallet/rpc/spend.cpp:206: * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
test/functional/rpc_estimatefee.py:39: # max value of 1008 per src/policy/fees.h
test/functional/rpc_psbt.py:604: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_basic.py:337: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_fundrawtransaction.py:851: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_send.py:315: expect_error=(-8, "Invalid conf_target, must be between 1 and 1008")) # max value of 1008 per src/policy/fees.h
```
This is fixed in this PR by running a script that searches for what he greps and replaces it with the right reference.
```
git grep -l "policy\/fees\.h" | xargs sed -i "s/policy\/fees.h/policy\/fees\/block_policy_estimator.h/g"
```
ACKs for top commit:
kevkevinpal:
ACK [b0a3887](b0a3887154)
janb84:
ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
rkrux:
lgtm ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
Tree-SHA512: e24f2aaf18fcfb0ae047a53ed209135a644ff08f5a8bc162c1522be3f99d7d01d550fc2e73d8db5fec7b748902daf68e61e7a5624f5913b9824feba5641fc78c
c25a5e670b27d3b6eb958ce437dbe89678bd1511 init: Signal m_tip_block_cv on Ctrl-C (Ryan Ofsky)
6a29f79006a9d60b476893dface5eea8f9bf271c test: Test SIGTERM handling during waitforblockheight call (Ryan Ofsky)
Pull request description:
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.
This issue was reported by plebhash in #33463. These hangs have been present since #30409. A similar bug was also fixed previously in Qt in #18452 and this PR simplifies that fix.
ACKs for top commit:
Sjors:
tACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
TheCharlatan:
ACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
enirox001:
Concept ACK c25a5e6
Tree-SHA512: 320aaa74fd308e826521c48c9a8aca4bd5f5530064cda2303d251d8e93e50c474bcd0db760ce04921928e73abefe4847aff797ac9ca7c89e74e5051bbed061cd
6eaa00fe20206baedc0d8ade5bb8d066ea615704 test: clarify submitBlock() mutates the template (Sjors Provoost)
862bd432837efeb6ab1435f75493501618ab3190 mining: ensure witness commitment check in submitBlock (Sjors Provoost)
00d1b6ef4b1203e80271c16c0d5b179525de1913 doc: clarify UpdateUncommittedBlockStructures (Sjors Provoost)
Pull request description:
When an IPC client requests a new block template via the Mining interface, we hold on to its `CBlock`. That way when they call `submitSolution()` we can modify it in place, rather than having to reconstruct the full block like the `submitblock` RPC does.
Before this commit however we forgot to invalidate `m_checked_witness_commitment`, which we should since the client brings a new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now reject such a block. As a sanity check, we add a second node to the test and confirm that will accept our mined block.
As first noticed in #33374 the IPC code takes the coinbase as provided, unlike the `submitblock` RPC which calls `UpdateUncommittedBlockStructures()` and adds witness commitment to the coinbase if it was missing.
Although that could have been an alternative fix, we instead document that IPC clients are expected to provide the full coinbase including witness commitment.
Patch to produce the original issue:
```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index b988e28a3f..28e9048a4d 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -450,15 +450,10 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
}
block.nVersion = version;
block.nTime = timestamp;
block.nNonce = nonce;
block.hashMerkleRoot = BlockMerkleRoot(block);
-
- // Reset cached checks
- block.m_checked_witness_commitment = false;
- block.m_checked_merkle_root = false;
- block.fChecked = false;
}
std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
KernelNotifications& kernel_notifications,
CTxMemPool* mempool,
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index cce56e3294..bf1b7048ab 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -216,22 +216,22 @@ class IPCInterfaceTest(BitcoinTestFramework):
assert_equal(res.result, True)
# The remote template block will be mutated, capture the original:
remote_block_before = await self.parse_and_deserialize_block(template, ctx)
- self.log.debug("Submitted coinbase must include witness")
+ self.log.debug("Submitted coinbase with missing witness is accepted")
assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex())
res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())
- assert_equal(res.result, False)
+ assert_equal(res.result, True)
self.log.debug("Even a rejected submitBlock() mutates the template's block")
# Can be used by clients to download and inspect the (rejected)
# reconstructed block.
remote_block_after = await self.parse_and_deserialize_block(template, ctx)
assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex())
- self.log.debug("Submit again, with the witness")
+ self.log.debug("Submit again, with the witness - does not replace the invalid block")
res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
assert_equal(res.result, True)
self.log.debug("Block should propagate")
assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height + 1)
```
ACKs for top commit:
ryanofsky:
Code review ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704. Just documentation updates and test clarifications since last review, also splitting up a commit.
TheCharlatan:
Re-ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704
ismaelsadeeq:
Code review and tested ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704
Tree-SHA512: 3a6280345b0290fe8300ebc63c13ad4058d24ceb35b7d7a784b974d5f04f420860ac03a9bf2fc6a799ef3fc55552ce033e879fa369298f976b9a01d72bd55d9e
8810642b571e1d8482375e962a1129b691d5d226 test: add option to skip large re-org test in feature_block (brunoerg)
Pull request description:
Fixes#32877
This PR adds a config flag `--skipreorg` which is used to skip the large re-org test. According to corecheck, `feature_block` is our slowest functional test and primarily because of this large re-org test. However, this test might not be useful for the mutation analysis of some files and could be skipped to save a huge amount of time.
```
time ./build/test/functional/feature_block.py --skipreorg
./build/test/functional/feature_block.py --skipreorg 11.38s user 0.33s system 37% cpu 31.422 total
time ./build/test/functional/feature_block.py
./build/test/functional/feature_block.py 25.87s user 3.53s system 56% cpu 52.317 total
```
ACKs for top commit:
maflcko:
review ACK 8810642b571e1d8482375e962a1129b691d5d226 🥁
enirox001:
tACK 8810642 – Ran tests with/without --skipreorg; saw ~40 % speedup; no regressions.
theStack:
Concept and code-review ACK 8810642b571e1d8482375e962a1129b691d5d226
glozow:
lgtm ACK 8810642b571e1d8482375e962a1129b691d5d226
Tree-SHA512: 4ef38bd32b8ad8ec2b7f30c96d2fe545d920759645ff52f632699f829b64f8d26fe878f3fdd255142235edd0a740a7feb64da8f5a10d0d740ebfa46c43ae60eb
060bb55508245776bb6a39c8b7849769ee588d69 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba547685ca4163316017ab78e965c7ad1 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db54166497685b80a12c51db6772982e01fe rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](060bb55508)
maflcko:
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
achow101:
ACK 060bb55508245776bb6a39c8b7849769ee588d69
rkrux:
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
01cc20f3307c532f402cdf5dc17f2f14920b725b test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07ed0a82386c1930e67683af5f2090e8b test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dcac0e1eeca2142a2d72d3d010335346f p2p: During block download, adjust pindexLastCommonBlock better (Martin Zumsande)
Pull request description:
As described in #32179, `pindexLastCommonBlock` is updated later than necessary
in master.
In case of a linear chain with no forks, it can be moved forward at the beginning of
`FindNextBlocksToDownload`, so that the updated value can be used to better estimate `nWindowEnd`.
This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.
I also changed `p2p_ibd_stalling.py` to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.
Fixes#32179
ACKs for top commit:
Crypt-iQ:
crACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
sipa:
utACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
79b4c276e7b9b526fa8f563b1e09b2b970baece6 Bugfix: QA: rpc_bind: Skip nonloopback test if no such address is found (Luke Dashjr)
Pull request description:
Without this, I get:
```
2025-09-19T03:14:05.157000Z TestFramework (INFO): PRNG seed is: 3218602557639511064
2025-09-19T03:14:05.158000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin-test/a
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for ipv6
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for non-loopback interface
2025-09-19T03:14:05.158000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.516000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.871000Z TestFramework (INFO): Bind test for ['[::1]']
2025-09-19T03:14:06.227000Z TestFramework (INFO): Bind test for ['127.0.0.1', '[::1]']
2025-09-19T03:14:06.583000Z TestFramework (INFO): Using interface None for testing
2025-09-19T03:14:06.583000Z TestFramework (INFO): Bind test for [None]
2025-09-19T03:14:06.583000Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 126, in run_test
self._run_nonloopback_tests()
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 157, in _run_nonloopback_tests
self.run_bind_test([self.non_loopback_ip], self.non_loopback_ip, [self.non_loopback_ip],
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[(self.non_loopback_ip, self.defaultport)])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 38, in run_bind_test
expected = [(addr_to_hex(addr), port) for (addr, port) in expected]
~~~~~~~~~~~^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/netutil.py", line 132, in addr_to_hex
if '.' in addr: # IPv4
^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable
```
ACKs for top commit:
maflcko:
review ACK 79b4c276e7b9b526fa8f563b1e09b2b970baece6 🏑
theStack:
Tested ACK 79b4c276e7b9b526fa8f563b1e09b2b970baece6
Tree-SHA512: 2a723d9bc5d1d50a8321a4f8a8cac3da3125d373ea71e6cc9d03de07307008f58970e361490d4c34530a6a976cb078b62d0ef09b7fb321ca1cfb9249a70d99a5
dcb56fd4cb59e6857c110dd87019459989dc1ec3 interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
ryanofsky:
Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
TheCharlatan:
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
ec8516ceb7568d7b09836b830023978bd37f8462 test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py (Sebastian Falbesoner)
Pull request description:
This small cleanup PR is a late follow-up to #31250 (commit c847dee1488a294c9a9632a00ba1134b21e41947). These helpers are unused and wouldn't work anymore, as they call a legacy wallet RPC (`dumpprivkey`). They were only ever used for testing the `importmulti` RPC, which also doesn't exist anymore. Functional tests that need to create key pairs and derive various output script types from them can use `get_generate_key` (introduced in #16528, commit f193ea889ddb53d9a5c47647966681d525e38368) instead, without involving the node.
ACKs for top commit:
rkrux:
crACK ec8516ceb7568d7b09836b830023978bd37f8462
brunoerg:
code review ACK ec8516ceb7568d7b09836b830023978bd37f8462
Tree-SHA512: cab3701f1a8fbcff0eecea4cfdc632ffac226afd2eefe3c9274a84ee1bb71fb231a57cd0876025c714be257a249157b048b67e309b3734442c425d85cf481cf6
These helpers use a legacy wallet RPC (`dumpprivkey`) and thus don't
work anymore. They were only ever used for testing the `importmulti`
RPC, which also doesn't exist anymore.
81e5c8385b9ec170c97190a97c560a39ccfc544a test: cover invalid codesep positions for signature in taproot (Greg Sanders)
Pull request description:
There is some basic coverage, but I felt like adding some boundary conditions where the only issue is the codesep value would be nice.
ACKs for top commit:
ajtowns:
ACK 81e5c8385b9ec170c97190a97c560a39ccfc544a
TheCharlatan:
ACK 81e5c8385b9ec170c97190a97c560a39ccfc544a
Tree-SHA512: de74895c3bb49854987654720ebcefea2f47c4a55ba6ab4a52878f6a9a0bd8b3085afa3485101610327fa8d35c3d074542f58540e126460bd4bea918cb0054ee
PR #33374 proposed a new Mining IPC method applySolution() which
could be used by clients to obtain the reconstructed block for
inspection, especially in the case of a rejected block.
However it was pointed out during review that submitBlock() modified
the template CBlock in place, so the client can just call getBlock()
and no new method is needed.
This commit adds a test to document that (now intentional) behavior.
When an IPC client requests a new block template via the Mining interface,
we hold on to its CBlock. That way when they call submitSolution() we can
modify it in place, rather than having to reconstruct the full block like
the submitblock RPC does.
Before this commit however we forgot to invalidate
m_checked_witness_commitment, which we should since the client brings a
new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now reject such a block.
As a sanity check, we add a second node to the test and confirm that will
accept our mined block.
Note that the IPC code takes the coinbase as provided, unlike the
submitblock RPC which calls UpdateUncommittedBlockStructures() and adds
witness commitment to the coinbase if it was missing.
Although that could have been an alternative fix, we instead document that
IPC clients are expected to provide the full coinbase including witness
commitment.
66667d6512294fd5dd02161b7c68c19af0865865 test: Use same rpc timeout for authproxy and cli (MarcoFalke)
Pull request description:
It seems odd to use different timeouts (and timeout factors) depending on whether the Python RPC proxy is used, or the bitcoin rpc command line interface.
Fix it by using the same timeout.
This can be tested by introducing a timeout error and checking it happens with and without `--usecli` after the exact same time.
Example timeout error:
```diff
diff --git a/test/functional/mining_template_verification.py b/test/functional/mining_template_verification.py
index de0833c596..e0f93a2b1e 100755
--- a/test/functional/mining_template_verification.py
+++ b/test/functional/mining_template_verification.py
@@ -173,7 +173,7 @@ class MiningTemplateVerificationTest(BitcoinTestFramework):
self.log.info("Submitting this block should succeed")
assert_equal(node.submitblock(block.serialize().hex()), None)
- node.waitforblockheight(2)
+ node.waitforblockheight(200000)
def transaction_test(self, node, block_0_height, tx):
self.log.info("make block template with a transaction")
```
Example cmd: `./bld-cmake/test/functional/mining_template_verification.py --timeout-factor=0.1 --usecli`.
ACKs for top commit:
brunoerg:
ACK 66667d6512294fd5dd02161b7c68c19af0865865
stickies-v:
tACK 66667d6512294fd5dd02161b7c68c19af0865865
Tree-SHA512: c8c21d8b9fb60ab192e3bbd45b317b96a40e10bf03704148613ac3cbdaae4abc2c03c4afbd504309ea0958201267c0d2a4bc5b40aa020917175c47e080ffe292
57f7c68821d96cc096db624cb06b7a252d038300 test: add functional test for `TestShell` (matching doc example) (Sebastian Falbesoner)
53874f7934d50f68ea46df68b2bcd11bad834730 doc: test: update TestShell example instructions/options (Sebastian Falbesoner)
Pull request description:
This PR adds a functional framework test for the `TestShell` class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in `test-shell.md` get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don't expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.
Alternatively, the example instructions in `test-shell.md` could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).
The first commit contain two small removal fix-ups in `test-shell.md` regarding the result of the `createwallet` RPC call and the mentioning of the `noshutdown` option that was removed earlier (see #31620). Could be backported to v30.
ACKs for top commit:
brunoerg:
ACK 57f7c68821d96cc096db624cb06b7a252d038300
stratospher:
ACK 57f7c68.
pinheadmz:
ACK 57f7c68821d96cc096db624cb06b7a252d038300
Tree-SHA512: 25c35af46b742bbefce7838708437529bbf613fa3d1f0fba590cacef0acdde82b3a78c7a01459c73eaac26ce3f1041e54092d098f0fc94a8c76ac0b4f4970338
664657ed134365588914c2cf6a3975ce368a4f49 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed134365588914c2cf6a3975ce368a4f49
rkrux:
lgtm crACK 664657ed134365588914c2cf6a3975ce368a4f49
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
0465574c127907df9b764055a585e8281bae8d1d test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e71d196120e6c9d0b1d1923a4927408d test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e6bdd590a9f6badd15d897b5ef5ce54 Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23aac64a37d929eeae81325e221d177d Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e0b04feca6ec09738ecbe792095a338 test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b471d07a2e8ee79edde46ec67f47d580 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c127907df9b764055a585e8281bae8d1d
TheCharlatan:
ACK 0465574c127907df9b764055a585e8281bae8d1d
furszy:
Tested ACK 0465574c127907df9b764055a585e8281bae8d1d.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
45bd8914658a675d00aa9c83373d6903a8a9ece8 log: split assumevalid ancestry-failure-reason message (Lőrinc)
6c13a38ab51caf1fa7502f746e33bbf86153a541 log: separate script verification reasons (Lőrinc)
f2ea6f04e79b6646b9320a910694a22c5520977d refactor: untangle assumevalid decision branches (Lőrinc)
9bc298556cb02cfa1382bbaa9e5638006b403576 validation: log initial script verification state (Lőrinc)
4fad4e992c49a532e3a8928965f242cb311eeb29 test: add assumevalid scenarios scaffold (Lőrinc)
91ac64b0a66fc792eabd0a9bb5bb22459c852c6d log: reword `signature validations` to `script verification` in `assumevalid` log (Lőrinc)
Pull request description:
### Summary
Users can encounter cases where script checks are unexpectedly enabled (e.g. after reindex, or when `assumevalid`/`minimumchainwork` gates fail). Without an explicit line, they must infer state from the absence of a message, which is incomplete and error-prone.
The existing "Assuming ancestors of block …" line does not reliably indicate whether script checks are actually enabled, which makes debugging/benchmarking confusing.
### What this changes
We make the initial **script-verification** state explicit and log **why** checks are enabled to avoid confusion.
* Always log the first script-verification state on startup, **before** the first `UpdateTip`.
* Flatten the nested `assumevalid` conditionals into a linear gating sequence for readability.
* Extend the functional test to assert the old behavior with the new reason strings.
This is a **logging-only** test change it shouldn't change any other behavior.
### Example output
The state (with reason) is logged at startup and whenever the reason changes, e.g.:
* `Disabling script verification at block #904336 (000000000000000000014106b2082b1a18aaf3091e8b337c6fed110db8c56620).`
* `Enabling script verification at block #912527 (000000000000000000010bb6aa3ecabd7d41738463b6c6621776c2e40dbe738a): block too recent relative to best header.`
* `Enabling script verification at block #912684 (00000000000000000001375cf7b90b2b86e559d05ed92ca764d376702ead3858): block height above assumevalid height.`
------
Follow-up to https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037
ACKs for top commit:
Eunovo:
re-ACK 45bd891465
achow101:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
hodlinator:
re-ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
yuvicc:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
andrewtoth:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
ajtowns:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
Tree-SHA512: 58328d7c418a6fe18f1c7fe1dd31955bb6fce8b928b0df693f6200807932eb5933146300af886a80a1d922228d93faf531145186dae55ad4ad1f691970732eca
fa37153288ca420420636046ef6b8c4ba7e5a478 util: Abort on failing CHECK_NONFATAL in debug builds (MarcoFalke)
fa0dc4bdffb06b6f0c192fe1aa02b4dfdcdc6e15 test: Allow testing of check failures (MarcoFalke)
faeb58fe668662d8262c4cc7c54ad2af756dbe3b refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD (MarcoFalke)
Pull request description:
A failing `CHECK_NONFATAL` will throw an exception. This is fine and even desired in production builds, because the program may catch the exception and give the user a way to easily report the bug upstream.
However, in debug development builds, exceptions for internal bugs are problematic:
* The exception could accidentally be caught and silently ignored
* The exception does not include a full stacktrace, possibly making debugging harder
Fix all issues by turning the exception into an abort in debug builds.
This can be tested by reverting the hunks to `src/rpc/node.cpp` and `test/functional/rpc_misc.py` and then running the functional or fuzz tests.
ACKs for top commit:
achow101:
ACK fa37153288ca420420636046ef6b8c4ba7e5a478
ryanofsky:
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review
stickies-v:
ACK fa37153288ca420420636046ef6b8c4ba7e5a478
Tree-SHA512: 2d892b838ccef6f9b25a066e7c2f6cd6f5acc94aad1d91fce62308983bd3f5c5d724897a76de4e3cc5c3678ddadc87e2ee8c87362965373526038e598dfb0101
d0e1bbad016cc4949094daea2934712f92dfeecd test: repeat block malleability test with relayable block over P2P (Musa Haruna)
Pull request description:
This PR adds a functional test to repeat the existing malleability check for oversized coinbase witness nonce size using a block that is small enough to be relayed over the P2P network.
This addresses the TODO in test_block_malleability by ensuring behavior is consistent between submitblock RPC and P2P relay.
ACKs for top commit:
maflcko:
lgtm ACK d0e1bbad016cc4949094daea2934712f92dfeecd
janb84:
re ACK d0e1bbad016cc4949094daea2934712f92dfeecd
glozow:
utACK d0e1bbad016cc4949094daea2934712f92dfeecd
Tree-SHA512: 05aec4fade5af8043f40274a8d2f3cf3f540acd038138975bdefbbbc81e105792d6d2588256a2ee5ddb1e05b37fe2d0b3d287160d2dbe86e1aac7cfa9cc02116
8f7673257a1a86717c1d83770dc857fc254df107 miner: fix empty mempool case for waitNext() (Sjors Provoost)
Pull request description:
Block template fees are calculated by looping over `new_tmpl->vTxFees` and return (early) once the `fee_threshold` is exceeded.
This left an edge case when the mempool is empty, which this commit fixes and adds a test for.
Also update `test/functional/interface_ipc.py` to reflect the new behavior,
Fixes https://github.com/Sjors/sv2-tp/issues/9
ACKs for top commit:
optout21:
ACK 8f7673257a1a86717c1d83770dc857fc254df107
cedwies:
tACK 8f76732
sipa:
utACK 8f7673257a1a86717c1d83770dc857fc254df107
zaidmstrr:
Concept ACK [8f76732](8f7673257a)
Tree-SHA512: ef200fe95e96f810e425283bc37f945c4bf5efa16f4b74820b8a07968f30c5146bca213a372124be84b48beead5dfd35f2b5d10d188d0a465f847ebab61de10a
e9cd45e3d3c7592265ebf67387090b3df1501df4 test: set number of RPC server threads to 2 (furszy)
Pull request description:
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily (more when tests are run
in parallel).
Furthermore, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
Note for reviewers:
I checked this does not introduce any timing regression but would be good
to double-check it on your end too. We could add another thread if needed.
Just the 16 threads default value is too high and unnecessary.
ACKs for top commit:
maflcko:
lgtm ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
l0rinc:
ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
kevkevinpal:
ACK [e9cd45e](e9cd45e3d3)
andrewtoth:
ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
Tree-SHA512: a777286f4a890fb87f5df72cd2ccfdc628657206a4b3e995044e5a0d12987b8c78a7cf7d684cc4e92605aa782aaeebc44e9f754752c3a524152fac94fa30f4b5
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily.
Moreover, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
ac599c4a9cb3b2d424932d3fd91f9eed17426827 test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c59802a6810a462eaa8dd61728ba820 wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0ec10f0c5ae5d94b9925fa334d1c6e6 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db938899409c8ee1cef04e16ba1795ea0038d sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f5004dc3df5a1b1d752114ba68fa5f4 sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56eac3a2e2b9ba489b5423d098cce0db sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607cde6187d7082429d27b927dc21c0c6 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679e16668458512aba2fd13a3e1bdb09f psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f53363f013ed3972997f0b05b9c19e9db9d signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86ff2347538e95041ab7b97af25342958 Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e494443cd82708490f384aa3034ad43bd sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9d6732aae3e69efd33105792ef1198f pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213aad0e67088b75cae24323db9f2576d8 musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e09f4e41802f07be53fb220d6f6c127f sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda644f1fc9a815b2d16c997716cd63554a tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7e38dd3f30b5d1a8f6b2fff0e380d12 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0ea87279558a8e6cbb3bccf5ba1bb781 descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
rkrux:
lgtm tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
theStack:
Code-review ACK ac599c4a9cb3b2d424932d3fd91f9eed17426827 🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
Block template fees are calculated by looping over new_tmpl->vTxFees
and return (early) once the fee_threshold is exceeded.
This left an edge case when the mempool is empty, which this commit
fixes and adds a test for. It does so by using std::accumulate instead
of manual loops.
Also update interface_ipc.py to account for the new behavior.
Co-authored-by: Raimo33 <claudio.raimondi@protonmail.com>
This test (which would fail without the previous commit) checks
that after the stalling block was received, we don't incorrectly
mark another peer as a staller immediately.
The `createwallet` RPC doesn't return the empty passphrase
warning anymore if no passphrase was passed explicitly.
The `noshutdown` parameter key was removed in commit
fa0dc09b9002f0bcae63af6af8d37fb3e0040ef4, so remove it from
the table.
652424ad162b63d73ecb6bd65bd26946e90c617f test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01ac014c57aca47f44d7f8d3da351987 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66efc39c6566ab31836e4eb582b77581d2 script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82daf3ed1441820a0ffc4c5ef78f64f1 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe060e7e582914dcb7acfaeee7a8ac48 script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e098c3f560b1ff50a37ebfef2af5f32 rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f073be799a35dfa92ab5004e12b35467 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37eba3ca6abc66386a3b9dc2185fa3ce Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
darosior:
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
theStack:
Code-review ACK 652424ad162b63d73ecb6bd65bd26946e90c617f 🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
dda5228e02ca6a839bf87ae7dbd133547563816a test: set par=2 in default config for functional test framework (Andrew Toth)
Pull request description:
Depending on the host machine, a default `par` value can spawn up to 15 script verification threads for each node. Running the functional test suite with default `par` can exhaust file descriptors or hit other resource limits when many threads are spawned. These threads are mostly idle and the same code paths are executed with a value of `par=2`. Limit this to 2 for functional tests that do not override the default option.
ACKs for top commit:
maflcko:
lgtm ACK dda5228e02ca6a839bf87ae7dbd133547563816a
pablomartin4btc:
ACK dda5228e02ca6a839bf87ae7dbd133547563816a
l0rinc:
Code review ACK dda5228e02ca6a839bf87ae7dbd133547563816a
theStack:
ACK dda5228e02ca6a839bf87ae7dbd133547563816a
Tree-SHA512: 4459972330ff50ac7391141db6382579de09d84e68959eaeb5f20972bb9daf9aac1bd68355028ded9ee65e838c12dbd53e6f3bb6cdc375d269f666c19a19eaec
14ae71f323dd011c6d51470ea15cf00750970f65 test: make notfound_on_unannounced more reliable (David Gumberg)
99bc552980d9a10da03e4b90c390bcd6cae686be test: fix (w)txid confusion in p2p_leak_tx.py (Martin Zumsande)
576dd97cb91ecff7d95898a0dc79b9b1a8a8f4f2 test: increase timeout in p2p_leak_tx.py (Martin Zumsande)
Pull request description:
This fixes two issues with `p2p_leak_tx.py`:
1.) #33090: As far as I can see, this is just the randomness of `NextInvToInbounds`/ `rand_exp_duration`, which has a probability of `e^-(60s/5s) = 6.14×10^−6` to result in a period > 60s (our waiting time), so that the test would fail every 160k runs... Doubling the timeout should be sufficient to lower the probability drastically.
2.) The subtest `test_notfound_on_unannounced_tx` has some (w)txid confusion: we send a `MSG_TX`-type getdata with a `wtxid` in it, which necessarily always results in a NOTFOUND. Fixed this, and change the subtest to be more deterministic based on `mocktime`.
ACKs for top commit:
stratospher:
ACK 14ae71f. nice restructuring using mocktime!
davidgumberg:
reACK 14ae71f323
vasild:
ACK 14ae71f323dd011c6d51470ea15cf00750970f65
Tree-SHA512: be5a4ca7bf56f82b6fa04d90ef9312dc2e6f8ff7ddf70b39d979dc42fbdd823157109b8b5dc46eb7f81ac1e816f40e6966b3c8a7d384aadee01e2189c20d3e3a
06df14ba75be5f48cf9c417424900ace17d1cf4d test: add more TRUC reorg coverge (Greg Sanders)
26e71c237d9d2197824b547f55ee3a0a60149f92 Mempool: Do not enforce TRUC checks on reorg (Greg Sanders)
bbe8e9063c15dc230553e0cbf16d603f5ad0e4cf fuzz: don't bypass_limits for most mempool harnesses (Greg Sanders)
Pull request description:
This was the intended behavior but our tests didn't cover the scenario where in-block transactions themselves violate TRUC topological constraints.
The behavior in master will potentially lead to many erroneous evictions during a reorg, where evicted TRUC packages may be very high feerate and make sense to mine all together in the next block and are well within the normal anti-DoS chain limits.
This issue exists since the merge of https://github.com/bitcoin/bitcoin/pull/28948/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R956
ACKs for top commit:
sdaftuar:
ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d
glozow:
ACK 06df14ba75b
ismaelsadeeq:
Code review ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d
Tree-SHA512: bdb6e4dd622ed8b0b11866263fff559fcca6e0ca1c56a884cca9ac4572f0026528a63a9f4c8a0660df2f5efe0766310a30e5df1d6c560f31e4324ea5d4b3c1a8
When the assumevalid ancestry check fails, log a precise reason:
- "block height above assumevalid height" if the block is above the assumevalid block (the default reason)
- "block not in of assumevalid chain" otherwise
The new split was added under the existing condition to simplify conceptually that the two cases are related.
It could still be useful to know when the block is just above the assumevalid block or when it's not even on the same chain.
Update the functional test to assert the new reason strings. No behavior change.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Replace `fScriptChecks` with `script_check_reason` and log the precise reason when checks are enabled; log a plain "Disabling" when they are skipped.
Adjust the functional test to assert the new reason strings.
Co-authored-by: w0xlt <woltx@protonmail.com>
Co-authored-by: Eunovo <eunovo9@gmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
