This makes the debug output mostly the same for -par=1 and parallel validation runs. Of course,
parallel validation is non-deterministic in what error it may encounter first if there are
multiple issues. Also, the way certain script-related and non-script-related checks are
performed differs between the two modes still, which may result in discrepancies.
409d0d6293 test: enable running individual independent functional test methods (ismaelsadeeq)
Pull request description:
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but having a pragmatic option to do this is more convenient.
Note: Running test methods that require arguments or context will fail.
**Example Usage**:
```zsh
build/test/functional/feature_reindex.py --test_methods continue_reindex_after_shutdown
```
```zsh
build/test/functional/feature_config_args.py --test_methods test_log_buffer test_args_log test_connect_with_seednode
```
ACKs for top commit:
maflcko:
review ACK 409d0d6293
rkrux:
reACK 409d0d6293
ryanofsky:
Code review ACK 409d0d6293. This seems like a good step towards making it easy to run independent tests quickly. I think ideally there would be some naming convention or @ annotation added to test methods that can run independently, so the test framework could provide more functionality like being able to list test methods, being able to show command lines to quickly reproduce problems when tests fails, and calling test methods automatically instead of requiring individual tests to call them. But these ideas are all compatible with the new `--test_methods` option
Tree-SHA512: b0daac7c3b322e6fd9b946962335d8279e8cb004ff76f502c8d597b9c4b0073840945be198a79d44c5aaa64bda421429829d5c84ceeb8c6139eb6ed079a35878
62f6d9e1a4 test: simple ordering optimization to reduce runtime (tdb3)
Pull request description:
Noticed in #31371 that the position of `mempool_ephemeral_dust` within `BASE_SCRIPTS` was lengthening total test runtime. Instead of moving only that test, looked for others to move to reduce runtime.
This is a quick optimization that was found to reduce overall functional test runtime of up to around 20% (depending on jobs and machine characteristics). Since it seems like test ordering could be done in many different ways, with many variables, and bike shedding could creep in, a relatively straightforward approach was taken for now that minimized changes to test_runner.
ACKs for top commit:
maflcko:
lgtm ACK 62f6d9e1a4
TheCharlatan:
ACK 62f6d9e1a4
Tree-SHA512: 6f93fbe4de3fce202383d9f84aa0e96961af3de3c02b8cab73589339d701f32c5e1b57a191eeebf4b06b5cd7a82617f63f24110732940be1a5a4d9237813a570
faa16ed4b9 test: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py (MarcoFalke)
Pull request description:
This was forgotten by myself in commit fa5b58ea01.
This time, there is a diff to test, which fails on current master and passes with this pull request.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e503a68382..16438ebd08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -112,9 +112,9 @@ static_assert(MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP, "MAX_BLOCKTXN_DEPTH too
* want to make this a per-peer adaptive value at some point. */
static const unsigned int BLOCK_DOWNLOAD_WINDOW = 1024;
/** Block download timeout base, expressed in multiples of the block interval (i.e. 10 min) */
-static constexpr double BLOCK_DOWNLOAD_TIMEOUT_BASE = 1;
+static constexpr double BLOCK_DOWNLOAD_TIMEOUT_BASE = .05; // 30 sec
/** Additional block download timeout per parallel downloading peer (i.e. 5 min) */
-static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.5;
+static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.;
/** Maximum number of headers to announce when relaying blocks with headers message.*/
static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
/** Minimum blocks required to signal NODE_NETWORK_LIMITED */
diff --git a/test/functional/p2p_ibd_stalling.py b/test/functional/p2p_ibd_stalling.py
index fa07873929..f8cdd8998c 100755
--- a/test/functional/p2p_ibd_stalling.py
+++ b/test/functional/p2p_ibd_stalling.py
@@ -82,6 +82,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
# Need to wait until 1023 blocks are received - the magic total bytes number is a workaround in lack of an rpc
# returning the number of downloaded (but not connected) blocks.
bytes_recv = 172761 if not self.options.v2transport else 169692
+ time.sleep(31);
self.wait_until(lambda: self.total_bytes_recv_for_blocks() == bytes_recv)
self.all_sync_send_with_ping(peers)
ACKs for top commit:
brunoerg:
ACK faa16ed4b9
Tree-SHA512: 5a670e2dcf828ac83b721a3e20d897744cca50080b0583a8460a0d0c7bf2c2c988cf7e35f688dde6a3349f1c21cc83a16ea5242ed06a59d59a04130416690737
160799d913 test: refactor: introduce `create_ephemeral_dust_package` helper (Sebastian Falbesoner)
61e18dec30 doc: ephemeral policy: add missing closing double quote (Sebastian Falbesoner)
Pull request description:
This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279:
https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825279952
Happy to add more if I missed some or anyone has concrete commits to add.
ACKs for top commit:
rkrux:
tACK 160799d913
instagibbs:
ACK 160799d913
tdb3:
Code review ACK 160799d913
Tree-SHA512: e9a80c6733f1e7fe9e834d81b404f6e8ef7a61fe986f61b3dcdbda1a0bc547145fc279ec02f54361df56cb4e62a6fedaa0f3991c6e084c3a703ed1b1bfbdbe4e
37a5c5d836 doc: update descriptors.md for getdescriptoractivity (James O'Beirne)
ee3ce6a4f4 test: rpc: add no address case for getdescriptoractivity (James O'Beirne)
811f76f3a5 rpc: add getdescriptoractivity (James O'Beirne)
25fe087de5 rpc: move-only: move ScriptPubKeyDoc to utils (James O'Beirne)
Pull request description:
The RPC command `scanblocks` provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (`relevant_blocks`). However actually extracting the activity from those blocks is left as an exercise to the end user.
This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via `deriveaddresses`), but then the user must retrieve each block's contents one-by-one using `getblock <hash>`, which is transmitted over a network link. And that's all before they perform the actual search over block content. There's even more work required to incorporate unconfirmed transactions.
This PR introduces an RPC `getdescriptoractivity` that [dovetails](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-08-16#1046393;) with `scanblocks` output, handling the process described above. Users specify the blockhashes (perhaps from `relevant_blocks`) and a set of descriptors; they are then given all spend/receive activity in that set of blocks.
This is a very useful tool when implementing lightweight wallets that want neither to require a third-party indexer like electrs, nor the overhead of creating and managing watch-only wallets in Core. This allows Core to be more easily used in a "stateless" manner by wallets, with potentially many nodes interchangeably acting as backends.
### Example usage
```
% ./src/bitcoin-cli scanblocks start \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' \
857263
{
"from_height": 857263,
"to_height": 858263,
"relevant_blocks": [
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
],
"completed": true
}
% ./src/bitcoin-cli getdescriptoractivity \
'["00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88", "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"]' \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]'
{
"activity": [
{
"type": "receive",
"amount": 0.00002900,
"blockhash": "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"height": 857907,
"txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"vout": 254,
"output_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
},
{
"type": "spend",
"amount": 0.00002900,
"blockhash": "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb",
"height": 858260,
"spend_txid": "7f61d1b248d4ee46376f9c6df272f63fbb0c17039381fb23ca5d90473b823c36",
"spend_vin": 0,
"prevout_txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"prevout_vout": 254,
"prevout_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
}
]
}
```
ACKs for top commit:
instagibbs:
reACK 37a5c5d836
achow101:
ACK 37a5c5d836
tdb3:
Code review and light retest ACK 37a5c5d836
rkrux:
re-ACK 37a5c5d836
Tree-SHA512: 04aa51e329c6c2ed72464b9886281d5ebd7511a8a8e184ea81249033a4dad535a12829b1010afc2da79b344ea8b5ab8ed47e426d0bf2eb78ab395d20b1da8dbb
ee1128ead8 doc: update stack-clash-protection comment re mingw-w64 (fanquake)
bf47448f15 test: drop check for Windows < 10 (fanquake)
35b898c47f release: target Windows 10 or later (fanquake)
398754e70b depends: target Windows 10 when building for mingw-w64 (fanquake)
Pull request description:
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.
We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.
Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
ACKs for top commit:
hodlinator:
cr-ACK ee1128ead8
davidgumberg:
ACK ee1128ead8
achow101:
ACK ee1128ead8
hebasto:
re-ACK ee1128ead8, only rebased, a commit message and a comment have been amended since my recent [review](https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160).
TheCharlatan:
ACK ee1128ead8
Tree-SHA512: 245e0bac3d63414d919a1948661fef4ff79359faaacaf19d64abd91cc62e822797fb1cf3379e340bfdf9a85c0b88fd99a90eda450dd4218b6213ab78aefb1374
This is not a pure refactor:
1. It slightly changes the log messages, as reflected in the test changes
2. It adds the IP address to all disconnect logging (when fLogIPs is set)
And under the hood suppoert single transactions
in AcceptPackage. This simplifies user experience
and paves the way for reducing number of codepaths
for transaction acceptance in the future.
Co-Authored-By: instagibbs <gsanders87@gmail.com>
Can be tested by running
```
$ sudo tcpdump -i eth0 host 11.22.33.44
```
and verifying that no packets appear in the tcpdump output.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This tests the new submitblock behaviour that is introduced in the
previous commit: Submitting a previously pruned block should persist the
block's data again.
The coinbase check is repeated again early during ProcessNewBlock.
Pre-checking it may also shadow more fundamental problems with a block.
In most cases the block header is checked first, before validating the
transactions. Checking the coinbase first therefore masks potential
issues with the header. Fix this by removing the pre-check.
The pre-check was likely introduced on top of
ada0caa165 to fix UB in
GetWitnessCommitmentIndex in case a block's transactions are empty. This
code path could only be reached because of the call to
UpdateUncommittedBlockStructures in submitblock, but cannot be reached
through net_processing.
Add some functional test cases to cover the previous conditions that
lead to a "Block does not start with a coinbase" json rpc error being
returned.
---
With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.
The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.
Recently added mempool_util implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba.
5736d1ddac tracing: pass if replaced by tx/pkg to tracepoint (0xb10c)
a4ec07f194 doc: add comments for CTxMemPool::ChangeSet (Suhas Daftuar)
83f814b1d1 Remove m_all_conflicts from SubPackageState (Suhas Daftuar)
d3c8e7dfb6 Ensure that we don't add duplicate transactions in rbf fuzz tests (Suhas Daftuar)
d7dc9fd2f7 Move CalculateChunksForRBF() to the mempool changeset (Suhas Daftuar)
284a1d33f1 Move prioritisation into changeset (Suhas Daftuar)
446b08b599 Don't distinguish between direct conflicts and all conflicts when doing cluster-size-2-rbf checks (Suhas Daftuar)
b53041021a Duplicate transactions are not permitted within a changeset (Suhas Daftuar)
b447416fdd Public mempool removal methods Assume() no changeset is outstanding (Suhas Daftuar)
2b30f4d36c Make RemoveStaged() private (Suhas Daftuar)
18829194ca Enforce that there is only one changeset at a time (Suhas Daftuar)
7fb62f7db6 Apply mempool changeset transactions directly into the mempool (Suhas Daftuar)
34b6c5833d Clean up FinalizeSubpackage to avoid workspace-specific information (Suhas Daftuar)
57983b8add Move LimitMempoolSize to take place outside FinalizeSubpackage (Suhas Daftuar)
01e145b975 Move changeset from workspace to subpackage (Suhas Daftuar)
802214c083 Introduce mempool changesets (Suhas Daftuar)
87d92fa340 test: Add unit test coverage of package rbf + prioritisetransaction (Suhas Daftuar)
15d982f91e Add package hash to package-rbf log message (Suhas Daftuar)
Pull request description:
part of cluster mempool: #30289
It became clear while working on cluster mempool that it would be helpful for transaction validation if we could consider a full set of proposed changes to the mempool -- consisting of a set of transactions to add, and a set of transactions (ie conflicts) to simultaneously remove -- and perform calculations on what the mempool would look like if the proposed changes were to be applied. Two specific examples of where we'd like to do this:
- Determining if ancestor/descendant/TRUC limits would be violated (in the future, cluster limits) if either a single transaction or a package of transactions were to be accepted
- Determining if an RBF would make the mempool "better", however that idea is defined, both in the single transaction and package of transaction cases
In preparation for cluster mempool, I have pulled this reworking of the mempool interface out of #28676 so it can be reviewed on its own. I have not re-implemented ancestor/descendant limits to be run through the changeset, since with cluster mempool those limits will be going away, so this seems like wasted effort. However, I have rebased #28676 on top of this branch so reviewers can see what the new mempool interface could look like in the cluster mempool setting.
There are some minor behavior changes here, which I believe are inconsequential:
- In the package validation setting, transactions would be added to the mempool before the `ConsensusScriptChecks()` are run. In theory, `ConsensusScriptChecks()` should always pass if the `PolicyScriptChecks()` have passed and it's just a belt-and-suspenders for us, but if somehow they were to diverge then there could be some small behavior change from adding transactions and then removing them, versus never adding them at all.
- The error reporting on `CheckConflictTopology()` has slightly changed due to no longer distinguishing between direct conflicts and indirect conflicts. I believe this should be entirely inconsequential because there shouldn't be a logical difference between those two ideas from the perspective of this function, but I did have to update some error strings in some tests.
- Because, in a package setting, RBFs now happen as part of the entire package being accepted, the logging has changed slightly because we do not know which transaction specifically evicted a given removed transaction.
- Specifically, the "package hash" is now used to reference the set of transactions that are being accepted, rather than any single txid. The log message relating to package RBF that happen in the `TXPACKAGES` category has been updated as well to include the package hash, so that it's possible to see which specific set of transactions are being referenced by that package hash.
- Relatedly, the tracepoint logging in the package rbf case has been updated as well to reference the package hash, rather than a transaction hash.
ACKs for top commit:
naumenkogs:
ACK 5736d1ddac
instagibbs:
ACK 5736d1ddac
ismaelsadeeq:
reACK 5736d1ddac
glozow:
ACK 5736d1ddac
Tree-SHA512: 21810872e082920d337c89ac406085aa71c5f8e5151ab07aedf41e6601f60a909b22fbf462ef3b735d5d5881e9b76142c53957158e674dd5dfe6f6aabbdf630b
111465d72d test: Remove unused attempts parameter from wait_until (Fabian Jahr)
5468a23eb9 test: Add check_interval parameter to wait_until (Fabian Jahr)
16c87d91fd test: Introduce ensure_for helper (Fabian Jahr)
Pull request description:
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to `wait_until`.
- This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
- I have also been going back and forth on naming. To me `ensure` works well but I am also not a native speaker, happy consider a different name if others don't think it's clear enough.
ACKs for top commit:
maflcko:
re-ACK 111465d72d🍋
achow101:
ACK 111465d72d
tdb3:
code review re ACK 111465d72d
furszy:
utACK 111465d72d
Tree-SHA512: ce01a4f3531995375a6fbf01b27d51daa9d4c3d7cd10381be6e86ec5925d2965861000f7cb4796b8d40aabe3b64c4c27e2811270e4e3c9916689575b8ba4a2aa
0bd53d913c test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c8 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783a validation: add RecalculateBestHeader() function (Martin Zumsande)
Pull request description:
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.
This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.
One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.
Fixes #26245
ACKs for top commit:
fjahr:
reACK 0bd53d913c
achow101:
ACK 0bd53d913c
TheCharlatan:
Re-ACK 0bd53d913c
Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
After port collisions are no longer tolerated but lead to
a startup failure in v28.0, local setups of multiple nodes,
each with a different -port value would not be possible anymore
due to collision of the onion default port - even if the nodes
were using tor or not interested in receiving onion inbound connections.
Fix this by deriving the onion listening port to be -port + 1.
(idea by vasild / laanwj)
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
The mempool:replaced tracepoint now reports either a txid or a
package hash (previously it always was a txid). To let users know
if a txid or package hash is passed, a boolean argument is added
the the tracepoint.
In the functional test, a ctypes.Structure class for MempoolReplaced
is introduced as Python warns the following when not explcitly
casting it to a ctype:
Type: 'bool' not recognized. Please define the data with ctypes manually.
5c2e291060 bench: Add basic CheckEphemeralSpends benchmark (Greg Sanders)
3f6559fa58 Add release note for ephemeral dust (Greg Sanders)
71a6ab4b33 test: unit test for CheckEphemeralSpends (Greg Sanders)
21d28b2f36 fuzz: add ephemeral_package_eval harness (Greg Sanders)
127719f516 test: Add CheckMempoolEphemeralInvariants (Greg Sanders)
e2e30e89ba functional test: Add ephemeral dust tests (Greg Sanders)
4e68f90139 rpc: disallow in-mempool prioritisation of dusty tx (Greg Sanders)
e1d3e81ab4 policy: Allow dust in transactions, spent in-mempool (Greg Sanders)
04b2714fbb functional test: Add new -dustrelayfee=0 test case (Greg Sanders)
Pull request description:
A replacement for https://github.com/bitcoin/bitcoin/pull/29001
Now that we have 1P1C relay, TRUC transactions and sibling eviction, it makes sense to retarget this feature more narrowly by not introducing a new output type, and simple focusing on the feature of allowing temporary dust in the mempool.
Users of this can immediately use dust outputs as:
1. Single keyed anchor (can be shared by multiple parties)
2. Single unkeyed anchor, ala P2A
Which is useful when the parent transaction cannot have fees for technical or accounting reasons.
What I'm calling "keyed" anchors would be used anytime you don't want a third party to be able to run off with the utxo. As a motivating example, in Ark there is the concept of a "forfeit transaction" which spends a "connector output". The connector output would ideally be 0-value, but you would not want that utxo spend by anyone, because this would cause financial loss for the coordinator of the service: https://arkdev.info/docs/learn/concepts#forfeit-transaction
Note that this specific use-case likely doesn't work as it involves a tree of dust, but the connector idea in general demonstrates how it could be used.
Another related example is connector outputs in BitVM2: https://bitvm.org/bitvm2.html .
Note that non-TRUC usage will be impractical unless the minrelay requirement on individual transactions are dropped in general, which should happen post-cluster mempool.
Lightning Network intends to use this feature post-29.0 if available: https://github.com/lightning/bolts/issues/1171#issuecomment-2373748582
It's also useful for Ark, ln-symmetry, spacechains, Timeout Trees, and other constructs with large presigned trees or other large-N party smart contracts.
ACKs for top commit:
glozow:
reACK 5c2e291060 via range-diff. Nothing but a rebase and removing the conflict.
theStack:
re-ACK 5c2e291060
Tree-SHA512: 88e6a6b3b91dc425de47ccd68b7668c8e98c5683712e892c588f79ad639ae95c665e2d5563dd5e5797983e7542cbd1d4353bc90a7298d45a1843b05a417f09f5
83fab3212c test: Add combinerawtransaction test to rpc_createmultisig (Ava Chow)
Pull request description:
The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.
Split from #28710
ACKs for top commit:
maflcko:
re-ACK 83fab3212c
BrandonOdiwuor:
Re-ACK 83fab3212c
Abdulkbk:
ACK 83fab3212c
brunoerg:
code review ACK 83fab3212c
rkrux:
tACK 83fab3212c
Tree-SHA512: 383d88ff6c9b54337ed81c714026e527b0fed41d976959fd5c6863b49d0defa4ea13fdc3d984885c86a2b6380825cd66c17842cc31f20fbec4bc42d86aecbbfa
- Some test methods in the functional test framework are independent
and do not require any previous context or setup defined in `run_test`.
- This commit adds a new option for running these specific methods within a test file,
allowing them to be executed individually without running the entire test suite.
- running test methods that require an argument or context will fail.
c189eec848 doc: release note for mempoolrullrbf removal (Greg Sanders)
d47297c6aa rpc: Mark fullrbf and bip125-replaceable as deprecated (Greg Sanders)
04a5dcee8a docs: remove requirement to signal bip125 (Greg Sanders)
111a23d9b3 Remove -mempoolfullrbf option (Greg Sanders)
Pull request description:
Given https://github.com/bitcoin/bitcoin/pull/30493 and the related discussion on network uptake it's probably not helpful to have an option for a feature that will not be respected by the network in any meaningful way.
Wallet changes can be done in another PR on its own cadence to account for possible fingerprinting, waiting for fullrbf logic to permeate the network, etc.
ACKs for top commit:
stickies-v:
re-ACK c189eec848
achow101:
ACK c189eec848
murchandamus:
ACK c189eec848
rkrux:
reACK c189eec848
Tree-SHA512: 9447f88f8f291c56c5bde70af0a91b0a4f5163aaaf173370fbfdaa3c3fd0b44120b14d3a1977f7ee10e27ffe9453f8a70dd38aad0ffb8c39cf145049d2550730
The only coverage of combinerawtransaction is in a legacy wallet only
test. So also use it in rpc_createmultisig so that this RPC remains
tested after the legacy wallet is removed.
d7fd766feb test: added test to assert TX decode rpc error on submitpackage rpc (kevkevinpal)
Pull request description:
This PR adds coverage for this line https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L996
If you run the following you will get no results for `submitpackage`
`grep -nri "TX decode failed" ./test/functional`
ACKs for top commit:
achow101:
ACK d7fd766feb
instagibbs:
reACK d7fd766feb
tdb3:
ACK d7fd766feb
rkrux:
reACK d7fd766feb
Tree-SHA512: e92e0e2621a4efab35625d8da3ac61ccb7fa65c378aa977112bc132fd3b42431f8c3ceb081f7c9903ed2833c229042b65bdb11444e1d6367354ae65dc7504231
bbbbaa0d9a Fix unsigned integer overflows in interpreter (MarcoFalke)
Pull request description:
Unsigned integer overflow is well defined by the language and in some cases even useful or necessary. However, I think that it should be avoided in interpreter, as it makes the code harder to read and requires the whole file to be suppressed in the sanitizer. This puts more burden on reviewers to check that any changes to interpreter that involve unsigned integer overflow are sane.
This patch involves a few changes:
* Evaluate the addition in 64-bit "space". Previously, the first argument was `size_t` (unsigned, 32-bit or 64-bit, depending on platform) and the second was `int` (32-bit on all supported platforms). Thus the addition was done in 32-bit or 64-bit "unsigned space". Now the addition is done in 64-bit "signed space" on all platforms. This is safe because signed integer overflow (UB) isn't expected here with 64-bit integers.
* Clarify that the value passed to the "stack macros" always fits in an `int64_t`. This is done with the C++11 syntax `int64_t{i}`, which fails to compile if `i` needs to be narrowed to fit into `int64_t`.
* Explicitly convert the result of the addition to `size_t`. This isn't needed, because the called function already converts the value (see https://en.cppreference.com/w/cpp/container/vector/operator_at), however I have a slight preference for the explicit cast. (Happy to remove if reviewers prefer without)
The patch does not change the bitcoind binary on my 64-bit system with `clang++ -O2`. However, it does change with gcc.
ACKs for top commit:
achow101:
ACK bbbbaa0d9a
ismaelsadeeq:
Code review ACK bbbbaa0d9a
hebasto:
ACK bbbbaa0d9a, I have reviewed the code and it looks OK.
Tree-SHA512: 0e9cbc6a0afd3db0d1d9489fd5e32ff856217604abde370add1f01c2cae8c526f2afedeb372997217c3a70ab0f8f56442e8230f87456f8e21c9abcb7c6578f7c
e60cecc811 doc: add release note for 31156 (Martin Zumsande)
fc7dfb3df5 test: Don't enforce BIP94 on regtest unless specified by arg (Martin Zumsande)
Pull request description:
The added arg `-test=bip94` is only used in a functional test for BIP94. This is done because the default regtest consensus rules should follow mainnet, not testnet.
Fixes#31137.
ACKs for top commit:
achow101:
ACK e60cecc811
tdb3:
cr and light test ACK e60cecc811
rkrux:
tACK e60cecc811
BrandonOdiwuor:
utACK e60cecc811
laanwj:
Code review ACK e60cecc811
Tree-SHA512: ca2f322f89d8808dfc3565fe020d2615cfcc110e188a02128ad7108fef51c735b33d55b5e6a70c505d78f7291f3c635dc7dfbcd78be1348d4d6e483883be4216
57529ac4db test: set P2PConnection.p2p_connected_to_node in peer_connect_helper() (Vasil Dimov)
22cd0e888c test: support WTX INVs from P2PDataStore and fix a comment (Vasil Dimov)
ebe42c00aa test: extend the SOCKS5 Python proxy to actually connect to a destination (Vasil Dimov)
ba621ffb9c test: improve debug log message from P2PConnection::connection_made() (Vasil Dimov)
Pull request description:
If requested, make the SOCKS5 Python proxy redirect connections to a set of given destinations. Actually act as a real proxy, connecting the client to a destination, except that the destination is not what the client asked for.
This would enable us to "connect" to Tor addresses from the functional tests.
Plus a few other minor improvements in the test framework as individual commits.
---
These changes are part of https://github.com/bitcoin/bitcoin/pull/29415 but they make sense on their own and would be good to have them, regardless of the fate of #29415. Also, if this is merged, that would reduce the size of #29415, thus the current standalone PR.
ACKs for top commit:
jonatack:
Approach ACK 57529ac4db
achow101:
ACK 57529ac4db
tdb3:
CR and test ACK 57529ac4db
mzumsande:
Code review / tested ACK 57529ac4db
Tree-SHA512: a2892c97bff2d337b37455c409c6136cb62423ce6cc32b197b36f220c1eec9ca046b599135b9a2603c0eb6c1ac4d9795e73831ef0f04378aeea8b245ea733399
0ea84bc362 test: explicitly check boolean verbosity is disallowed (tdb3)
7a2e6b68cd doc: add rpc guidance for boolean verbosity avoidance (tdb3)
698f302df8 rpc: disallow boolean verbosity in getorphantxs (tdb3)
63f5e6ec79 test: add entry and expiration time checks (tdb3)
808a708107 rpc: add entry time to getorphantxs (tdb3)
56bf302714 refactor: rename rpc_getorphantxs to rpc_orphans (tdb3)
7824f6b077 test: check that getorphantxs is hidden (tdb3)
ac68fcca70 rpc: disallow undefined verbosity in getorphantxs (tdb3)
Pull request description:
Implements follow-up suggestions from #30793.
- Now disallows undefined verbosity levels (below and above valid values) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
- Disallows boolean verbosity (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) and adds guidance to developer-notes
- Checks that `getorphantxs` is a hidden rpc (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
- Adds a test for `expiration` time
- Adds `entry` time to the returned orphan objects (verbosity >=1) to relieve the user from having to calculate it from `expiration`. Also adds associated test. (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
- Minor cleanup (blank line removal and log message move) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)
Included a commit to rename the test to a more generic `get_orphans` to better accommodate future orphanage-related RPCs (e.g. `getorphanangeinfo`). Can drop the refactor commit from this PR if people feel strongly about it.
ACKs for top commit:
achow101:
ACK 0ea84bc362
glozow:
utACK 0ea84bc362
rkrux:
tACK 0ea84bc362
itornaza:
tACK 0ea84bc362
Tree-SHA512: e48a088f333ebde132923072da58e970461e74362d0acebbc799c3043d5727cdf5f28e82b43cb38bbed27c603df6710695dba91ff0695e623ad168e985dce08e
c4dc81f9c6 test: Remove dead code from interface_zmq (Fabian Jahr)
Pull request description:
The loop removed here appears to be effectively dead code: In case `get_raw_seq` is behind `zmq_mem_seq` the loop runs and tries to get a more recent (higher) number for `get_raw_seq`. However, the exact number of `get_raw_seq` is asserted in the line above: `assert_equal(get_raw_seq, 6)`. If the loop would actually achieve its purpose this assert would need to be racy. This does not seem to be the case and 6 appears to be the final number. `zmq_mem_seq` however does take some time to catch up (if it were continue to be updated). But this is not handled by the loop and does not seem to be relevant at this point in the test. The backlog is consumed a bit later in another loop that handles this correctly already.
ACKs for top commit:
l0rinc:
ACK c4dc81f9c6
tdb3:
CR re ACK c4dc81f9c6
Tree-SHA512: 663a1711ba1ce04a3d2e2916e0df7a7bb51069e28bc2644b816a483628c95b5e6c29fc6eacc31a5f72b7d9af11096f3c437ea1dc57eaa1ee9ddce43cc20bacd3
6c9fe7b73e test: Prevent connection attempts to random IPs in p2p_seednodes.py (Martin Zumsande)
bb97b1ffa9 test: fix intermittent timeout in p2p_seednodes.py (Martin Zumsande)
Pull request description:
Fixes#31103
On some CI runs, the seed node timer in `ThreadOpenConnection` was only started *after* the mocktime was set.
Fix this by waiting for the first connection attempt, which happens after the timer was started.
Also I noticed that the "unreachable" connections are not in fact unreachable, so that the functional test could attempt connections
to random IPs on the internet. This was already noted in https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 but the suggested fix never made it in, so I added it to this PR.
ACKs for top commit:
sr-gi:
tACK [6c9fe7b](6c9fe7b73e)
laanwj:
Code review ACK 6c9fe7b73e
tdb3:
cr and light test ACK 6c9fe7b73e
Tree-SHA512: 021b6d5325eab85d79708b4b137f61723a36f2b8a1faf681463bad2ea5283ea528b5ff1701467a86b035d3a6972750a61ace5020e58b7aa61ecaad97664488c8
The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.
The use of `PACKAGE_NAME` for the project's variable name is
problematic, as this name is commonly used in CMake's interface
variables. If third-party CMake code handles with scopes improperly,
our `PACKAGE_NAME` variable could end up with an unexpected value.
This change avoids such conflicts by renaming all `PACKAGE_*` variables
to `CLIENT_*`.
40e5f26a3f mapport: remove dead code in DispatchMapPort (Antoine Poinsot)
38fdf7c1fb mapport: drop outdated comments (Antoine Poinsot)
b7b2435290 doc: add release note for #31130 (Antoine Poinsot)
1b6dec98da depends: drop miniupnpc (Antoine Poinsot)
953533d021 doc: remove mentions of UPnP (Antoine Poinsot)
94ad614482 ci: remove UPnP options (Antoine Poinsot)
a9598e5eaa build: drop miniupnpc dependency (Antoine Poinsot)
a5fcfb7385 interfaces: remove now unused 'use_upnp' arg from 'mapPort' (Antoine Poinsot)
038bbe7b20 daemon: remove UPnP support (Antoine Poinsot)
844770b05e qt: remove UPnP settings (Antoine Poinsot)
Pull request description:
This PR removes UPnP IGD support and drops our [miniupnp](https://github.com/miniupnp/miniupnp) dependency.
Miniupnpc is a C library (somewhat) maintained by a single person which had several vulnerabilities in the past (a couple dozens are listed [here](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=miniupnp)), some of which directly affected our software ([RCE in 2015](https://bitcoincore.org/en/2024/07/03/disclose_upnp_rce/), [OOM in 2020](https://bitcoincore.org/en/2024/07/31/disclose-upnp-oom/)).
The main purpose of this functionality is to have more (non-data-center) reachable nodes on the network. For a non-technical user running Bitcoin Core at home, the software would automatically open a port on their router to receive incoming connections. This way, users not able to manually open a port on their router would still provide the network with more resources and enhance its diversity.
However, due to past vulnerabilities (and a worry about unknown future ones) in miniupnpc this feature was disabled by default in https://github.com/bitcoin/bitcoin/pull/6795. Having it disabled by default kills (most of?) the purpose of having this functionality in the first place: someone technical enough to understand the `-upnp` startup option or the "enable UPnP" setting is most likely able to open a port on his box in the first place.
In addition, laanwj implemented PCP with a NAT-PMP fallback directly in Bitcoin Core in https://github.com/bitcoin/bitcoin/pull/30043. If we ever want to re-enable automatic NAT traversal by default in Bitcoin Core, this is the best option (and in my opinion the only sane one). The NAT-PMP fallback makes it so compatibility shouldn't be (much of) an issue.
On balance, i believe that keeping this functionality and this barely maintained C dependency has higher costs than benefits. Therefore i propose that we get rid of it.
ACKs for top commit:
jarolrod:
ACK 40e5f26a3f
1440000bytes:
Code Review ACK 40e5f26a3f
laanwj:
Code review ACK 40e5f26a3f
i-am-yuvi:
Tested ACK 40e5f26a3f
Tree-SHA512: 9ea48662775510f5ec6de7af65790f7c8d211603398e9d8c634a86387be81b28081419a95b4d6680d3d7fe6a9f16cec99f16516548201dc7e49781909899a657
f32c34d0c3 functional test: Additional package evaluation coverage (Greg Sanders)
Pull request description:
Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted.
Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.
in response to https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637 where if applied onto that PR's old commit, the test fails due to package failure.
ACKs for top commit:
sdaftuar:
re-ACK f32c34d0c3
rkrux:
tACK f32c34d0c3
glozow:
reACK f32c34d0c3
Tree-SHA512: 739fcc5e66878b3def9b25dc588d8cb5349aaaa0901b11475879a413a03f6ea0e87d19de5bc4fb44ddd0436fdc052cdc3ed564f7e2ad510269aab9732d5c24eb
5c299ecafe test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped (kevkevinpal)
Pull request description:
After joining the bitcoin pr review club about https://github.com/bitcoin/bitcoin/pull/30793
I learned about [`CVE-2012-3789`](https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L4693)
So I was motivated to write a functional test that covers this part of the code,
This test should add the max number of orphans to a nodes orphanage and then attempt to add another, then asserts that the number of orphans is still at the max amount
ACKs for top commit:
achow101:
ACK 5c299ecafe
rkrux:
ACK 5c299ecafe
instagibbs:
ACK 5c299ecafe
tdb3:
ACK 5c299ecafe
Tree-SHA512: 687bba337978e0945e94af71632998221e5565a5d83cf5a59ecf2ee52c7262d8ff907b94dceea3b80bed441dd19b24790b2904e88e1da14d30827c5469fcb4d3
Current test coverage doesn't ensure that mempool trimming
doesn't appear prior to the entire package, and not just
the subpackage, is finished being submitted.
Add a scenario that covers this case, where package
ancestors can make it in individually, but would be
immadiately evicted if not for the package CPFP.
33a28e252a Change default help arg to `-help` and mention `-h` and `-?` as alternatives (Lőrinc)
f0130ab1a1 doc: replace `-?` with `-h` for bench_bitcoin help (Lőrinc)
Pull request description:
The question mark is interpreted as a wildcard for any single character in Zsh (see https://www.techrepublic.com/article/globbing-wildcard-characters-with-zsh), so `bench_bitcoin -?` will not show the help message on systems using Zsh, such as macOS.
Since `-h` provides equivalent help functionality (as defined in https://github.com/bitcoin/bitcoin/blob/master/src/common/args.cpp#L684-L693), the `benchmarking.md` documentation has been updated to ensure compatibility with macOS.
----
### -?
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -?
zsh: no matches found: -?
### -h
> % cmake -B build -DBUILD_BENCH=ON && cmake --build build && build/src/bench/bench_bitcoin -h
Usage: bench_bitcoin [options]
Options:
...
----
Based on the comments the args help default was also changed to `-help`, mentioning `-h` and `-?` (instead of `-?` being the default)
ACKs for top commit:
edilmedeiros:
tACK 33a28e252a
maflcko:
lgtm ACK 33a28e252a
achow101:
ACK 33a28e252a
rkrux:
tACK 33a28e252a
laanwj:
Code review ACK 33a28e252a
Tree-SHA512: 8c6e27488462be9ba9186b34abe6249c1d93026b3963acc0f42c75496f39407563766ae518cf1839156039cc0047e29d91f70d191cfb97e0fbde85665e88c71e
Keep the "-upnp" option as a hidden arg for one major version in order
to show a more user friendly error to people who had this option set in
their config file.
This aims to complete our test framework BDB parser to reflect
our read-only BDB parser in the wallet codebase. This could be
useful both for making review of #26606 easier and to also possibly
improve our functional tests for the BDB parser by comparing with
an alternative implementation.
31cc5006c3 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e833 init: Improve chainstate init db error messages (TheCharlatan)
cd093049dd init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f85d7 init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce880a3 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842ff1 init: Remove unneeded argument for mempool_opts checks (stickies-v)
Pull request description:
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
ACKs for top commit:
achow101:
ACK 31cc5006c3
mzumsande:
Code Review / lightly tested ACK 31cc5006c3
BrandonOdiwuor:
Code Review ACK 31cc5006c3.
stickies-v:
ACK 31cc5006c3
Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b
04e4d52420 test: add test for specifying custom pidfile via `-pid` (Sebastian Falbesoner)
b832ffe044 refactor: introduce default pid file name constant in tests (tdb3)
Pull request description:
This small PR adds test coverage for the `-pid` command line option, which allows to overrule the pid filename (`bitcoind.pid` by default). One can specify either a relative path (within the datadir) or an absolute one; the latter is tested using `self.options.tmpdir`. Note that the functional test file `feature_init.py` so far only contained a stress test; with this new sub-test added, both the description and the test name are adapted to be more generic.
ACKs for top commit:
achow101:
ACK 04e4d52420
tdb3:
ACK 04e4d52420
ryanofsky:
Code review ACK 04e4d52420
naiyoma:
Tested ACK [04e4d52420)
Tree-SHA512: b2bc8a790e5d187e2c84345f344f65a176b62caecd9797c3b9edf10294c741c33a24e535be640b56444b91dcf9c65c7dd152cdffd8b1c1d9ca68e5e3c6ad1e99
These addrs aren't unreachable as the test claims.
Specify a (non-working) proxy to make sure the connections fails
even if the addr was reachable.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
On some CI runs, the timer in ThreadOpenConnection was only started *after*
the mocktime was set. Fix this by waiting for the first connection attempt,
which happens after the timer was started.
Also convert some comments into log messages/add a log, so that the test
isn't completely silent.
Set `P2PConnection.p2p_connected_to_node` in
`P2PConnection.peer_connect_helper()` instead of
`TestNode.add_p2p_connection()` and
`TestNode.add_outbound_p2p_connection()`.
This way tests can create an instance of `P2PConnection` and use
`P2PConnection.peer_connect_helper()` directly.
If requested, make the SOCKS5 Python proxy redirect each connection to a
given destination. Actually act as a real proxy, connecting the
client to a destination, except that the destination is not what the
client asked for.
This would enable us to "connect" to Tor addresses from the functional
tests.
86e2a6b749 [test] A non-standard transaction which is also consensus-invalid should return the consensus error (Antoine Poinsot)
f859ff8a4e [validation] Improve script check error reporting (dergoegge)
Pull request description:
An input script might be invalid for multiple reasons. For example, it might fail both a standardness check and a consensus check, which can lead to a `mandatory-script-verify-flag-failed` error being reported that includes the script error string from the standardness failure (e.g. `mandatory-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)`), which is confusing.
ACKs for top commit:
darosior:
re-ACK 86e2a6b749
ariard:
Re-Code Review ACK 86e2a6b7
instagibbs:
ACK 86e2a6b749
Tree-SHA512: 053939107c0bcd6643e9006b2518ddc3a6de47d2c6c66af71a04e8af5cf9ec207f19e54583b7a056efd77571edf5fd4f36c31ebe80d1f0777219c756c055eb42
fac6cfe5ac lint: commit-script-check.sh: echo to stderr (MarcoFalke)
Pull request description:
This makes it easier to redirect the produced `git diff` on failure. On success, it shouldn't hurt, because the same output is still present, just on stderr.
Can be tested by introducing a fault in any scripted diff and then calling `commit-script-check.sh HEAD~..HEAD > any_file.txt`. Previously the file contained the full output, now it contains just the diff.
ACKs for top commit:
TheCharlatan:
ACK fac6cfe5ac
Tree-SHA512: b4dfad10a4a902729a7ad7533ed0ef86b9e79761083f2ec623d448a551462b268fe04bdba387ca62160dae9ef7b1781e005dec60f18b111d9bfa6b97357108e6
cd0edf26c0 tracing: cast block_connected duration to nanoseconds (0xb10c)
Pull request description:
When the `validation:block_connected` tracepoint was introduced in 8f37f5c2a5, the connect block duration was passed in microseconds `µs`. By starting to use steady clock in fabf1cdb20 this changed to nanoseconds `ns`. As the test only checked if the duration value is `> 0` as a plausibility check, this went unnoticed. This was detected this when setting up monitoring for block validation time as part of the Great Consensus Cleanup Revival discussion.
This change casts the duration explicitly to nanoseconds, updates the documentation, and adds a check for an upper bound to the tracepoint interface tests. The upper bound is quite lax as mining the block takes much longer than connecting the empty test block. It's however able to detect a duration passed in an incorrect unit (1000x off).
A previous version of this PR casted the duration to microseconds `µs` - however, as the last three major releases have had the duration as nanoseconds (and this went unnoticed), we assume that this is the API now and changeing it back to microseconds would break the API again. See also https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2067867597
ACKs for top commit:
maflcko:
re-lgtm ACK cd0edf26c0
laanwj:
re-ACK cd0edf26c0
Tree-SHA512: 54a1eea0297e01c07c2d071ffafbf97dbd080f763e1dc0014ff086a913b739637c1634b1cf87c90b94a3c2f66006acfaada0414a15769cac761e03bc4aab2a77
fa43c4f93c test: Print CompletedProcess object on error (MarcoFalke)
Pull request description:
It would be good to know the output on `Error parsing command output`. Otherwise test failures are meaningless: https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2325911157
Fix it by just printing the full `CompletedProcess` object.
Also, use the modern `subprocess.run` to simplify the code.
ACKs for top commit:
BrandonOdiwuor:
Code Review ACK fa43c4f93c
laanwj:
This contains some useful information, so ACK fa43c4f93c
Tree-SHA512: ae7c1cb1f48af2a6feae6d1a5a967c0720f6c6675c1ce20ace7cac18c00f3d4069b8abcc58204855e92ff5303158b9a942bab3b71acae0737768d941a5773c91
e64b2f1a16 doc: cmake: prepend and explain "build/" where needed (Larry Ruane)
Pull request description:
This is a small follow-on to #30741, prepend `build/` to the path for `test_runner.py`.
ACKs for top commit:
jonatack:
ACK e64b2f1a16
maflcko:
lgtm ACK e64b2f1a16
tdb3:
re ACK e64b2f1a16
Tree-SHA512: 80943d4f342987bf060adacb1c7db2e9ff8de5a6da592846ba23f230281d3a5b306162c4c86e61739a29323eaa4abf09f69f41302996d5809f448e5788a74a87
fae44c83da test: Remove 0.16.3 test from wallet_backwards_compatibility.py (MarcoFalke)
Pull request description:
The test checks that any wallet created with current master can not be loaded with `v0.16.3`. This is interesting documentation, however it is probably not something to keep as a test, because:
* It seems like an extremely unlikely (and unsupported) edge case that someone creates a wallet with master and then goes ahead to open it with a long EOL software version.
* A better test would be the inverse: Create a wallet with `v0.16.3` and open it with current master. This is already tested in `wallet_upgradewallet.py`, where I've added an additional balance check before upgrading the `v0.16.3` wallet.
* The test is intermittently failing when shutting down the `v0.16.3` node, for example in https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2357565564. The exact cause is unclear, but given that the test isn't worthy to keep, removing it will ensure that the error disappears.
ACKs for top commit:
Sjors:
utACK fae44c83da
fanquake:
ACK fae44c83da - I agree that test seems to have past it's usefulness, and the fact that it otherwise causes intemittent issues is further reason to remove it.
Tree-SHA512: 85bf428e616e0880198c1a7529936520505d7fa87c2eeb87a0457f13b50a163accaf5f80f9364dea978f6bd14b0b5350cda88f49aa7584682c8b5e0b0b117724
fa6d14eacb test: Treat exclude list warning as failure in CI (MarcoFalke)
Pull request description:
An outdated exclude list or otherwise an error in the exclude list handling is usually a bug.
So make it fatal in the CI, instead of silently ignoring it.
Fixes https://github.com/bitcoin/bitcoin/pull/30872/files#r1757015334
Can be tested with something like (with and without `--ci`):
```
./bld-cmake/test/functional/test_runner.py wallet_disable -x wallet_disablee
ACKs for top commit:
tdb3:
ACK fa6d14eacb
ismaelsadeeq:
utACK fa6d14eacb
Tree-SHA512: 03a70dff9d1272d982591d60ab764f9233d4802488bc1bad305a2755e2d7ed86e691ee94767a3bc5f68321b63214aba44e6f9edd1543dfad7a20f9397cf78734
98c1536852 test: add getorphantxs tests (tdb3)
93f48fceb7 test: add tx_in_orphanage() (tdb3)
34a9c10e8c rpc: add getorphantxs (tdb3)
f511ff3654 refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2 refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852
hodlinator:
re-ACK 98c1536852
danielabrozzoni:
ACK 98c1536852
pablomartin4btc:
tACK 98c1536852
itornaza:
reACK 98c1536852
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
fa2b7d8d6b Remove redundant unterminated-logprintf tidy check (MarcoFalke)
bbbb2e43ee log: Enforce trailing newline, Remove redundant m_started_new_line (MarcoFalke)
Pull request description:
There are many problems around missing a trailing newline while logging:
* All log lines are currently terminated by a trailing newline. This means any runtime code trying to handle a "missing" newline is currently dead code.
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmaintained (no one updated it for the new log function names), probably harder to maintain than normal C++ code (because it requires clang AST matcher knowledge), brittle (it can fail to detect issues at any time, if it goes out-of-sync, or be explicitly disabled via `NOLINT`), and annoying for devs (it is slow and intricate to run locally and thus only effectively run on CI or via the CI scripts).
Fix all issues by enforcing the trailing newline in logs directly in the code. Then remove all the other stuff.
This refactor does not change behavior.
ACKs for top commit:
stickies-v:
re-ACK fa2b7d8d6b
achow101:
ACK fa2b7d8d6b
ryanofsky:
Code review ACK fa2b7d8d6b. Just comment and test cleanup since last review
Tree-SHA512: 10ed420f6c2fdb0f491d6c880be8dd2e8beef628f510adebadf4c3849d9f5e28906519d5cbaeb295f4c7c1b07c4c88a9905b3cfe30fee3a2c91ac9fd24ae6755
The weight unit is merely a consensus rule detail and is largely
irrelevant for fee-rate calculations and mempool policy rules (e.g. for
package relay and TRUC limits), so there doesn't seem to be any value of
using a granularity that we can't even guarantee to reach exactly
anyway.
Switch to the more natural unit of vsize instead, which simplifies both
the padding implementation and the current tests that take use of this
padding. The rather annoying multiplications by `WITNESS_SCALE_FACTOR`
can then be removed and weird-looking magic numbers like `4004` can be
replaced by numbers that are more connected to actual policy limit
constants from the codebase, e.g. `1001` for exceeding
`TRUC_CHILD_MAX_VSIZE` by one.
f5a2000579 test: re-bucket long-running tests (willcl-ark)
Pull request description:
Re-bucket:
- `p2p_node_network_limited -v*transport`
- `feature_assume_utxo`
On CI runners these tests are taking longer than their current bucket suggests, often being among the last to finish.
Re-bucket them to improve CI efficiency.
ACKs for top commit:
maflcko:
review ACK f5a2000579
Tree-SHA512: 3da5c888db64a311276338270ba1dcad3eb2a24e205f6bb86fc92f767ecfa63682f13fafffff569fa0cfaea607ccb538f31e3934a086d482c3fe1be5d39f8791
faf801515f test: Add missing sync_mempools() to fill_mempool() (MarcoFalke)
fa48be6f02 test: Refactor fill_mempool to extract send_batch helper (MarcoFalke)
Pull request description:
Not doing the sync will lead to (intermittent) issues, as explained in https://github.com/bitcoin/bitcoin/issues/30922#issuecomment-2364529013.
Fix all issues by doing the sync by default and disable it in places that do not need the sync.
Fixes#30922
ACKs for top commit:
mzumsande:
Tested ACK faf801515f
ismaelsadeeq:
Tested ACK faf801515f
marcofleon:
Tested ACK faf801515f
Tree-SHA512: 2de62d168cbb6857a9fb8bc12c42a9093fedf5e9beb6f83a32b3fa72a5ba3cf03631055fd25ef553399a27a6fe0d71c44cfe60660a4d31986debd13b8ab00228
f20fe33e94 test: Add basic balance coverage to wallet_assumeutxo.py (Fabian Jahr)
037b101e80 test: Add coverage for best block locator write in wallet_backup (Fabian Jahr)
31c0df0389 wallet: migration, write best locator before unloading wallet (furszy)
7e3dbe4180 wallet: Write best block to disk before backup (Fabian Jahr)
Pull request description:
I discovered that we don't write the best block to disk when trying to explain the behavior described here: https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882
In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in `wallet_assumeutxo.py` doesn't actually test what it says. It only fails because the best block isn't written and actually, the height of the backup that is loaded is at the snapshot height during backup. So it really shouldn't fail since it's past the background sync blocks already.
I'm not sure if this is super relevant in practice though so I am first looking for concept ACKs on the `BackupWallet` code change. Either way, I think this behavior should be documented better if it is left as is and the test should be changed.
ACKs for top commit:
achow101:
ACK f20fe33e94
furszy:
ACK f20fe33
Tree-SHA512: bb384a940df5c942fffe2eb06314ade4fc5d9b924012bfef3b1c456c4182a30825d1e137d8ae561d93d3a8a2f4d1c1ffe568132d20fa7d04844f1e289ab4a28b
7942951e3f Remove unused g_best_block (Ryan Ofsky)
e3a560ca68 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c Remove unused CRPCSignals (Sjors Provoost)
dca923150e Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121 rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05 Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf160 node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbb refactor: rename BlockKey to BlockRef (Sjors Provoost)
Pull request description:
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
`waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
There's a commit to add (direct) tests for the methods that are about to be refactored:
- `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
- `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`
This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.
The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).
The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.
`RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.
Finally `g_best_block` is also dropped.
ACKs for top commit:
achow101:
ACK 7942951e3f
ryanofsky:
Code review ACK 7942951e3f. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3f
Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
- p2p_node_network_limited -v*transport
- feature_assume_utxo
On CI runners these tests are taking longer than their current bucket
suggests, often being among the last to finish.
Re-bucket them to improve CI efficiency.
e6994efe08 fix: increase rpcbind check robustness (tdb3)
d38e3aed89 fix: handle invalid rpcbind port earlier (tdb3)
83b67f2e6d refactor: move host/port checking (tdb3)
73c243965a test: add tests for invalid rpcbind ports (tdb3)
Pull request description:
Previously, when an invalid port was specified in `-rpcbind`, the `SplitHostPort()` return value in `HTTPBindAddresses()` was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).
This rearranges port checking code in `AppInitMain()` to handle the invalid port before reaching `HTTPBindAddresses()`. Also adds a check in `HTTPBindAddresses()` as a defensive measure for future changes.
Adds then updates associated functional tests as well.
ACKs for top commit:
achow101:
ACK e6994efe08
ryanofsky:
Code review ACK e6994efe08
zaidmstrr:
Code review ACK [e6994ef](e6994efe08)
Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92
54227e681a rpc, cli: improve error message on multiwallet mode (pablomartin4btc)
Pull request description:
Running a CLI command when multiple wallets are loaded and `-rpcwallet` is not specified, should return a clearer error.
Currently in `master`:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).
Try adding "-rpcwallet=<filename>" option to bitcoin-cli command line.
```
With this change:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<walletname> URI path. Or for the CLI, specify the "-rpcwallet=<walletname>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).
```
ACKs for top commit:
maflcko:
review ACK 54227e681a
achow101:
ACK 54227e681a
furszy:
utACK 54227e681a
mzumsande:
Code Review ACK 54227e681a
jonatack:
ACK 54227e681a
Tree-SHA512: 51ff24f64858aa6be6adf6f20105c9f076ebea743780bf2a4399f7fe8b5239cbb1ea06d32b2ef5e850da2369abb0ef7a52c50c2b8f31f4ca90d3a486abc9b77e
facbcd4cef log: Use ConstevalFormatString (MarcoFalke)
fae9b60c4f test: Use LogPrintStr to test m_log_sourcelocations (MarcoFalke)
fa39b1ca63 doc: move-only logging warning (MarcoFalke)
Pull request description:
This changes all logging (including the wallet logging) to produce a
`ConstevalFormatString` at compile time, so that the format string can be
validated at compile-time.
I tested with `clang` and found that the compiler will use less than 1% more of time and memory.
When an error is found, the compile-time error depends on the compiler, but it may look similar to:
```
src/util/string.h: In function ‘int main(int, char**)’:
src/bitcoind.cpp:265:5: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>(((const char*)"Hi %s %s"))’
src/util/string.h:38:98: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(std::basic_string_view<char>(((const char*)((util::ConstevalFormatString<1>*)this)->util::ConstevalFormatString<1>::fmt)))’
src/util/string.h:78:34: error: expression ‘<throw-expression>’ is not a constant expression
78 | if (num_params != count) throw "Format specifier count must match the argument count!";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This refactor does not change behavior of the compiled executables.
ACKs for top commit:
hodlinator:
re-ACK facbcd4cef
l0rinc:
ACK facbcd4cef
ryanofsky:
Code review ACK facbcd4cef
pablomartin4btc:
re-ACK facbcd4cef
stickies-v:
Approach ACK and code LGTM facbcd4cef modulo a `tinyformat::format_error` concern.
Tree-SHA512: 852f74d360897020f0d0f6e5064edc5e7f7dacc2bec1d5feff22c634a2fcd2eb535aa75be0b7191d9053728be6108484c737154b02d68ad3186a2e5544ba0db8
a9964c0444 doc: Updating docs from autotools to cmake (kevkevinpal)
Pull request description:
A bit of a followup from https://github.com/bitcoin/bitcoin/pull/30840
- In this change the documentation where we refer to the `./configure` script which is now gone and have converted the configure params to use the `cmake` equivalent.
ACKs for top commit:
maflcko:
ACK a9964c0444
jonatack:
utACK a9964c0444
jarolrod:
ACK a9964c0444
tdb3:
ACK a9964c0444
pablomartin4btc:
re-ACK a9964c0444
Tree-SHA512: f7ed20b8ad61f028c0d242b9cc70650d8da63057d4a8f7da88f0117a8d3241c5fe8fcf19d56ec82088160b9fee9b175fe9f64e5a845260d3696dc7e94bfdd0bd
replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF
Previously, when an invalid port was specified
in `-rpcbind`, the `SplitHostPort()` return value
in `HTTPBindAddresses()` was ignored and attempt
would be made to bind to the default rpcbind port
(with the host/port treated as a host).
This rearranges port checking code in
`AppInitMain()` to handle the invalid
port before reaching `HTTPBindAddresses()`.
Also adjusts associated functional tests.
The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.
This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
This changes all logging (including the wallet logging) to produce a
ConstevalFormatString at compile time, so that the format string can be
validated at compile-time.
Also, while touching the wallet logging, avoid a copy of the template
Params by using const Params&.
This test would fail to mark the chain tip as "invalid" instead
of "headers-only" without the previous commit marking the headers
as BLOCK_FAILED_CHILD.
Without doing so, header-only chains building on a chain that
will be marked as invalid would still be eligible for m_best_header.
This improves both getblockchaininfo and getchaintips behavior.
While this adds an iteration over the entire block index, it can only be
triggered by the user (invalidateblock) or by others at a cost (the
header needs to be accepted in the first place, so it needs valid PoW).
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This means that it is being called in two situations:
1.) As part of the invalidateblock rpc
2.) When we receive a block for which we have a valid
header in our block index, but the block turns out to be invalid
6a1aa510e3 rpc: check block index before reading block / undo data (Martin Zumsande)
6cbf2e5f81 rpc: Improve gettxoutproof error when only header is available. (Martin Zumsande)
69fc867ea1 test: add coverage to getblock and getblockstats (Martin Zumsande)
5290cbd585 rpc: Improve getblock / getblockstats error when only header is available. (Martin Zumsande)
e5b537bbdf rest: improve error when only header of a block is available. (Martin Zumsande)
Pull request description:
Fixes#20978
If a block was pruned, `getblock` already returns a specific error: "Block not available (pruned data)".
But if we haven't received the full block yet (e.g. in a race with block downloading after a new block was received headers-first, or during IBD) we just return an unspecific "Block not found on disk" error and log
`ERROR: ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0) `
which suggest something went wrong even though this is a completely normal and expected situation.
This PR improves the error message and stops calling `ReadRawBlockFromDisk()`, when we already know from the header that the block is not available on disk.
Similarly, it prevents all other rpcs from calling blockstorage read functions unless we expect the data to be there, so that `LogError()` will only be thrown when there is an actual file system problem.
I'm not completely sure if the cause is important enough to change the wording of the rpc error, that some scripts may rely on.
If reviewers prefer it, an alternative solution would be to keep returning the current "Block not found on disk" error, but return it immediately instead of calling `ReadRawBlockFromDisk`, which would at least prevent the log error and also be an improvement in my opinion.
ACKs for top commit:
fjahr:
re-ACK 6a1aa510e3
achow101:
ACK 6a1aa510e3
andrewtoth:
re-ACK 6a1aa510e3
Tree-SHA512: 491aef880e8298a05841c4bf8eb913ef84820d1ad5415fd17d9b441bff181959ebfdd432b5eb8347dc9c568433f9a2384ca9d84cd72c79d8a58323ca117538fe
72c9a1fe94 test: Check that network stays suspended after dumptxoutset if it was off before (Fabian Jahr)
Pull request description:
Follow-up to #30817 which covered the robustness of `dumptxoutset`: network is deactivated during the run but re-activated even when an issue was encountered. But it did not cover the case if the user had deactivated the network themselves before. In that case the user may want the network to stay off so the network is not reactivated after `dumptxoutset` finishes. A test for this behavior is added here.
ACKs for top commit:
achow101:
ACK 72c9a1fe94
pablomartin4btc:
ACK 72c9a1fe94
theStack:
utACK 72c9a1fe94
tdb3:
tested ACK 72c9a1fe94
Tree-SHA512: 18a57c5782e99a018414db0597e88debeb32666712c2a75dddbb55135d8f1ddd1eeacba8bbbd35fc03b6c4ab0522fe074ec08edea729560b018f51efabf00e89
This avoids low-level log errors that are supposed to only occur when
there is an actual problem with the block on disk missing unexpectedly,
but not in the case where the block and/or undo data are expected not to be there.
It changes behavior such that in the first case (block index indicates
data is available but retrieving it fails) an error is thrown.
It also adjusts a functional tests that tried to simulate not
having undo data (but having block data) by deleting the undo file.
This situation should occur reality because block and undo data are pruned together.
Instead, test this situation with a block that hasn't been connected.
This improves the error message of the getblock and getblockstats rpc and prevents calls to
ReadRawBlockFromDisk(), which are unnecessary if we know
from the header nStatus field that the block is not available.
19f4a7c95a test: Wait for local services to update in feature_assumeutxo (Fabian Jahr)
Pull request description:
Closes#30878
It seems like there is a race where the block is stored locally and `getblock` does not error anymore, but `ActivateBestChain` has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.
Can be reproduced locally by adding the sleep here:
```cpp
──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
src/validation.cpp:3567: bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< │
──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
}
if (WITH_LOCK(::cs_main, return m_disabled)) {
std::this_thread::sleep_for(std::chrono::seconds(10));
// Background chainstate has reached the snapshot base block, so exit.
// Restart indexes to resume indexing for all blocks unique to the snapshot
```
ACKs for top commit:
maflcko:
review-only ACK 19f4a7c95a
achow101:
ACK 19f4a7c95a
pablomartin4btc:
tACK 19f4a7c95a
furszy:
Code review ACK [19f4a7c](19f4a7c95a).
Tree-SHA512: 70dad3795988956c5e20f2d2d895fb56c5e3ce257c7547d3fd729c88949f0e24cb34594da1537bce8794ad02b2db44e7e46e995aa32539cd4dd84c4f1d4bb1c4
fa5bc450d5 util: Use compile-time check for LogConnectFailure (MarcoFalke)
fa7087b896 util: Use compile-time check for FatalErrorf (MarcoFalke)
faa62c0112 util: Add ConstevalFormatString (MarcoFalke)
fae7b83eb5 lint: Remove forbidden functions from lint-format-strings.py (MarcoFalke)
Pull request description:
The `test/lint/lint-format-strings.py` was designed to count the number of format specifiers and assert that they are equal to the number of parameters passed to the format function. The goal seems reasonable, but the implementation has many problems:
* It is written in Python, meaning that C++ code can not be parsed correctly. Currently it relies on brittle regex and string parsing.
* Apart from the parsing errors, there are also many logic errors. For example, `count_format_specifiers` allows a mix of positional specifiers and non-positional specifiers, which can lead to runtime format bugs. Also, `count_format_specifiers` silently skipped over "special" format specifiers, which are valid in tinyformat, which again can lead to runtime format bugs being undetected.
* The brittle logic has a history of breaking in pull requests that are otherwise fine. This causes the CI to fail and the pull request being blocked from progress until the bug in the linter is fixed, or the code is rewritten to work around the bug.
* It is only run in the CI, or when the developer invokes the script. It would be better if the developer got the error message at compile-time, directly when writing the code.
Fix all issues by using a `consteval` checked format string in `FatalErrorf` and `LogConnectFailure`.
This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places.
ACKs for top commit:
stickies-v:
re-ACK fa5bc450d5
l0rinc:
ACK fa5bc450d5
hodlinator:
ACK fa5bc450d5
ryanofsky:
Code review ACK fa5bc450d5
Tree-SHA512: d6189096b16083143687ed1b1559cf4f92f97dd87bc5d00673e44f4fb9fce7bb7b215cfdfc39b6e6a24f0b75a79a03ededce966639e554f7172e1fc22cf015ae
72b46f28bf test: fix exclude parsing for functional runner (Max Edwards)
Pull request description:
This restores previous behaviour of being able to exclude a test by name without having to specify .py extension.
It was noticed in https://github.com/bitcoin/bitcoin/issues/30851 that tests were no longer being excluded.
PR https://github.com/bitcoin/bitcoin/pull/30244 introduced being able to exclude a specific tests based on args (such as `--exclude "rpc_bind.py --ipv6`) but it made the wrong assumption that test names intended to be excluded would include the .py extension.
The following https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 shows that this is not how the `--exclude` flag was used in CI.
https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 gave three examples of `--exclude` being used in CI so I compared the number of tests that the runner would run for these three examples in three situations, before #30244 was introduced, in master today and with this PR applied.
Example:
`--previous-releases --coverage --extended --exclude feature_dbcrash`
Test count:
Before #30244 introduced: 314
Master: 315
With this PR: 314
Example:
`--exclude feature_init,rpc_bind,feature_bind_extra`
Test count:
Before #30244 introduced: 306
Master 311
With this PR: 306
Example:
`--exclude rpc_bind,feature_bind_extra`
Before #30244 introduced: 307
Master 311
With this PR: 307
I've also tested that the functionality introduced with #30244 remains and we can still exclude specific tests by argument.
ACKs for top commit:
maflcko:
review ACK 72b46f28bf
willcl-ark:
ACK 72b46f28bf
Tree-SHA512: 37c0e3115f4e3efdf9705f4ff8cd86a5cc906aacc1ab26b0f767f5fb6a953034332b29b0667073f8382a48a2fe9d649b7e60493daf04061260adaa421419d8c8
2ad560139b Remove unused src_dir param from run_tests (Lőrinc)
Pull request description:
The `src_dir` usage was removed in a8a2e364ac (diff-437d7f6e9f2229879b60aae574a8217f14c643bbf3cfa9225d8011d6d52df00cL598), making the parameter unused.
Top commit has no ACKs.
Tree-SHA512: 1fd8b93811b4ab467ba5a160a4fe204e9606e1bf237c7595ed6f8b7821cf59d2a776c0e1e154852a45b2a35e5bdbd8996314e4f63a9c750f21b9a17875cb636a
992f83bb6f test: add coverage for assumeUTXO honest peers disconnection (furszy)
6d5812e5c8 assumeUTXO: fix peers disconnection during sync (furszy)
Pull request description:
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (`NODE_NETWORK`), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness from the AssumeUTXO
node.
This lack of response occurs because nodes ignore `getdata` requests when they do
not have the block data available (further discussion can be found in #30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal `NODE_NETWORK_LIMITED` support. Then, full-node (`NODE_NETWORK`)
support will be re-enabled once the background chain sync is completed.
Thanks mzumsande for a post-#30385 convo too.
Testing notes:
Just cherry-pick the second commit (bb08c22) on master.
It will fail there, due to the IBD node requesting historical blocks to the snapshot
node - which is bad because the snapshot node will ignore the requests and
stall + disconnect after some time.
ACKs for top commit:
achow101:
ACK 992f83bb6f
naumenkogs:
ACK 992f83bb6f
mzumsande:
ACK 992f83bb6f
Tree-SHA512: fef525d1cf3200c2dd89a346be9c82d77f2e28ddaaea1f490a435e180d1a47a371cadea508349777d740ab56e94be536ad8f7d61cc81f6550c58b609b3779ed3
082779d606 test: Add explicit onion bind to p2p_permissions (Ava Chow)
Pull request description:
When the bind option is replaced in the bitcoin.conf, bitcoind will attempd to bind to the default tor listening port. If another bitcoind is running that is already bound to that port, the bind will fail which, since #22729, causes the test to fail.
This failure can be avoided by explicitly binding the tor port when the bind is removed.
ACKs for top commit:
tdb3:
ACK 082779d606
theStack:
re-ACK 082779d606
glozow:
ACK 082779d606
Tree-SHA512: 4acb69ea2e00aeacf9e7c9ab9595ceaf0e0d2adbd795602034b2184197d9bad54c7bc9f3da43ef9c52a71869fe96ba8c87fc5b7c37880f258f5a2aaab2b4046c
Exercising and verifying the following points:
1. An IBD node can sync headers from an AssumeUTXO node at
any time.
2. IBD nodes do not request historical blocks from AssumeUTXO
nodes while they are syncing the background-chain.
3. The assumeUTXO node dynamically adjusts the network services
it offers according to its state.
4. IBD nodes can fully sync from AssumeUTXO nodes after they
finish the background-chain sync.
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
When the bind option is replaced in the bitcoin.conf, bitcoind will
attempd to bind to the default tor listening port. If another bitcoind
is running that is already bound to that port, the bind will fail which,
since #22729, causes the test to fail.
This failure can be avoided by explicitly binding the tor port when the
bind is removed.
c2b779da4e refactor: Manage dumptxoutset RAII classes with std::optional (Fabian Jahr)
4b5bf335ad test: Add coverage for failing dumptxoutset behavior (Fabian Jahr)
Pull request description:
This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117. To test the test you can comment out the content of the destructor of `NetworkDisable`.
It also addresses the feedback by ryanofsky to use `std::optional` instead of `std::unique_ptr` for the management of the RAII object: https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228
ACKs for top commit:
achow101:
ACK c2b779da4e
pablomartin4btc:
cr & tACK c2b779da4e
tdb3:
ACK c2b779da4e
BrandonOdiwuor:
Code Review ACK c2b779da4e
theStack:
ACK c2b779da4e
Tree-SHA512: 9556e75014a2599bb870b70faf887608b332f2312626333f771d4ec11c04f863a2cf17e223ec473d4e8b0c9e8008394a4e0c321561f7ef3a2eec713dcfaea58a
ee47ca29d6 init: fix fatal error on '-wallet' negated option value (furszy)
Pull request description:
Currently, if users provide a double negated value such as '-nowallet=0' or a non-boolean
convertible value to a negated option such as '-nowallet=not_a_boolean', the initialization
process results in a fatal error, causing an unclean shutdown and displaying a poorly
descriptive error message:
"JSON value of type bool is not of expected type string." (On bitcoind. The GUI
does not display any error msg - upcoming PR -).
This PR fixes the issue by ensuring that only string values are returned in the
the "wallet" settings list, failing otherwise. It also improves the clarity of the
returned error message.
Note:
This bug was introduced in https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was
replaced by `GetSettingsList("-wallet")`.
ACKs for top commit:
achow101:
ACK ee47ca29d6
ryanofsky:
Code review ACK ee47ca29d6, just adding the suggested test since last review
TheCharlatan:
ACK ee47ca29d6
ismaelsadeeq:
Tested ACK ee47ca29d6
Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd
27c976d11a fix: increase consistency of rpcauth parsing (tdb3)
2ad3689512 test: add norpcauth test (tdb3)
67df0dec1a test: blank rpcauth CLI interaction (tdb3)
ecc98ccff2 test: add cases for blank rpcauth (tdb3)
Pull request description:
The current `rpcauth` parsing behavior is inconsistent and unintuitive (see https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1972085251 and additional details below).
The current behavior inconsistently treats empty `rpcauth` as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid `rpcauth` params.
Empty `rpcauth` is now consistently treated as an error and prevents bitcoind from starting.
Continuation of the upforgrabs PR #29141.
### Additional details:
Current `rpcauth` behavior is nonsensical:
- If an empty `rpcauth` argument was specified as the last command line argument, it would cause all other `rpcauth` arguments to be ignored.
- If an empty `rpcauth` argument was specified on the command line followed by any nonempty `rpcauth` argument, it would cause an error.
- If an empty `rpcauth=` line was specified after non-empty rpcauth line in the config file it would cause an error.
- If an empty `rpcauth=` line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were `-rpcauth` command line arguments and the last one was nonempty, in which case it would cause an error.
New behavior is simple:
- If an empty rpcauth config line or command line argument is used it will cause an error
ACKs for top commit:
naiyoma:
Tested ACK [27c976d11a)
achow101:
ACK 27c976d11a
ryanofsky:
Code review ACK 27c976d11a. Since last review commit message was just tweaked to clarify previous behavior.
Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
Following changes were made:
1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved 'InterpretSubtractFeeFromOutputInstructions()' code organization.
5) Added test coverage for all possible error failures.
Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
- PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()
fadbcd51fc bench: Remove redundant logging benchmarks (MarcoFalke)
fa8dd952e2 bench: Use LogInfo instead of the deprecated alias LogPrintf (MarcoFalke)
Pull request description:
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
ACKs for top commit:
stickies-v:
ACK fadbcd51fc
Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
In case of a failure to create the dump, the node should not be left in an inconsistent state like deactivated network activity or an invalidated blockchain.
LogPrint*ThreadNames is redundant with LogWith(out)ThreadNames, because
they all measure toggling the thread names (and check that it has no
effect on performance).
This also allows to remove unused and deprecated macros.
8131bf7483 ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa0 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483
achow101:
ACK 8131bf7483
hebasto:
ACK 8131bf7483, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
fafdb7df34 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
7777047835 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f0 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69 test: [refactor] Fix E714 pycodestyle (MarcoFalke)
Pull request description:
The checks have many issues:
* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds
ACKs for top commit:
davidgumberg:
review and tested reACK fafdb7df34
kevkevinpal:
ACK [fafdb7d](fafdb7df34)
achow101:
ACK fafdb7df34
Tree-SHA512: a0488b722cfaf7071bd6848cd3be002e0b6c38af80d8b5cbb08613c0b174ef63277289f960db8ac31adb09fe563a4973203b8fb10b83cbcfdc6f0ef39bd04410
c8e6771af0 test: restrict multiple CLI arguments (naiyoma)
8838c4f171 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af0
tdb3:
cr re ACK c8e6771af0
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
ec317bc44b test: update satoshi_round function (naiyoma)
Pull request description:
This PR refactors `satoshi_round` to accept different rounding modes and make rounding a required argument.
Continuation of https://github.com/bitcoin/bitcoin/pull/23225
ACKs for top commit:
maflcko:
review ACK ec317bc44b
achow101:
ACK ec317bc44b
AngusP:
ACK ec317bc44b
Tree-SHA512: 070f0aa6f66e58bff7412cae6b71f5f4ab8c718c7b5eeba4bb604fe22c6416a1ced0474294f504e1c28943ddc073104466b5944b43bae27e42bee5ca85afa468
fb6d51eb25 signet/miner: Use argparse exclusive groups (Anthony Towns)
338a266a9a signet/miner: add support for a poolnum/poolid tag in mined blocks (Anthony Towns)
409ab7d35b signet/miner: add Generate.mine function (Anthony Towns)
7b31332370 signet/miner: add Generate.gbt function (Anthony Towns)
85c5c0bea9 signet/miner: add Generate.next_block_time function (Anthony Towns)
5540e6ca49 signet/miner: move next_block_* functions into new Generator class (Anthony Towns)
35f4631196 signet/miner: rename do_decode_psbt to decode_psbt (Anthony Towns)
aac040b439 signet/miner: drop create_coinbase function (Anthony Towns)
16951f549e signet/miner: drop do_createpsbt function (Anthony Towns)
3aed0a4284 signet/miner: drop get_reward_address function (Anthony Towns)
Pull request description:
Refactors the code a bunch, and adds `--poolnum` / `--poolid` options so that signers can tag their coinbases in a way that explorers can recognise (see also https://github.com/bitcoin-data/mining-pools/pull/82 and https://github.com/mempool/mempool/issues/2903).
The refactoring in particular helps enable the "try using inquisition's getblocktemplate, and if that doesn't work fall back to core's getblocktemplate" logic, as described/implemented in https://github.com/bitcoin-inquisition/bitcoin/pull/7
ACKs for top commit:
achow101:
ACK fb6d51eb25
danielabrozzoni:
Code review ACK fb6d51eb25
Tree-SHA512: d84095c4045ab196685b847e04ce2cdaedf387bc2527430ede918318dc5b70bf3d87b754264016f895f506fac70d4fdea5ef3cd8c3c375fd586afeae01e045e5
6eeb188d40 test: adds seednode functional tests (Sergi Delgado Segura)
3270f0adad net: Favor peers from addrman over fetching seednodes (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node
- Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary
This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts
ACKs for top commit:
achow101:
ACK 6eeb188d40
cbergqvist:
ACK 6eeb188d40
itornaza:
Tested ACK 6eeb188d40
tdb3:
ACK 6eeb188d40
Tree-SHA512: b04445412f22018852d6bef4d3f1e88425ee6ddb434f61dcffa9e0c41b8e31f8c56f83858d5c7686289c86dc4c9476c437df15ea61a47082e2bb2e073cc62f15
94b0adcc37 rpc, refactor: Prevent potential race conditions in dumptxoutset (Fabian Jahr)
e868a6e070 doc: Improve assumeutxo guide and add more docs/comments (Fabian Jahr)
b29c21fc92 assumeutxo: Remove devtools/utxo_snapshot.sh (Fabian Jahr)
20a1c77aa7 contrib: Remove test_utxo_snapshots.sh (Fabian Jahr)
8426850352 test: Test for dumptxoutset at specific height (Fabian Jahr)
993cafe7e4 RPC: Add type parameter to dumptxoutset (Fabian Jahr)
fccf4f91d2 RPC: Extract ReconsiderBlock helper (Fabian Jahr)
446ce51c21 RPC: Extract InvalidateBlock helper (Fabian Jahr)
Pull request description:
This adds a height parameter to the `dumptxoutset` RPC. This internalizes the workflow that was previously done by scripts: roll back the chain to the height we actually want the snapshot from, create the snapshot, roll forward to the real tip again.
The nice thing about internalizing this functionality is that we can write tests for the code and it gives us more options to make the functionality robust. The shell scripts we have so far will be more cumbersome to maintain in the long run, especially since we will only notice later when we have broken them. I think it's safe to remove these `test_utxo_snapshots.sh` as well when we have this option in `dumptxoutset` because we have also added some good additional functional test coverage for this functionality.
ACKs for top commit:
Sjors:
re-utACK 94b0adcc37
achow101:
ACK 94b0adcc37
mzumsande:
ACK 94b0adcc37
pablomartin4btc:
re-ACK 94b0adcc37
Tree-SHA512: a4c9af5f687d1ca7bfb579a36f363882823386b5fa80c05de531b05a2782b5da6ff5baf3ada4bca8f32f63975d86f1948175abed9affe51fc958472b5f838dab
When the tracepoint was introduced in 8f37f5c2a5,
the connect_block duration was passed in microseconds `µs`.
By starting to use steady clock in fabf1cdb20
this changed to nanoseconds `ns`. As the test only checked if the
duration value is `> 0` as a plausibility check, this went unnoticed.
I detected this when setting up monitoring for block validation time
as part of the Great Consensus Cleanup Revival discussion.
This change casts the duration explicitly to nanoseconds (as it has been
nanoseconds for the last three releases; switching back now would 'break'
the broken API again; there don't seem to be many users affected), updates
the documentation and adds a check for an upper bound to the tracepoint
interface tests. The upper bound is quite lax as mining the block takes
much longer than connecting the empty test block. It's however able to
detect incorrect duration units passed.
This is used in both cases - TCP server (accept) and TCP client (connect).
The message "Connected & Listening address:port" is confusing.
Print both ends of the TCP connection.
fa247e6e8c test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py (MarcoFalke)
Pull request description:
Similar to https://github.com/bitcoin/bitcoin/pull/30705:
The goal of this test case is to check that the sync works at all, not to check any timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 4110 BLOCKS_TO_MINE may take longer than the block download timeout.
Fix it by pinning the time using mocktime temporarily, and advance it immediately after the sync.
ACKs for top commit:
stratospher:
ACK fa247e6. Checked the timeout downloading block logs before/after using `setmocktime`.
tdb3:
ACK fa247e6e8c
Tree-SHA512: f61632a8d9e484f1b888aafbf87f7adf71b8692387bd77f603cdbc0de49f30d42e654741d46ae1ff8b9706a5559ee0faabdb192ed0db7449010b68bfcdbaa42d
fa09cb41f5 refactor: Remove unused LogPrint (MarcoFalke)
3333415890 scripted-diff: LogPrint -> LogDebug (MarcoFalke)
Pull request description:
`LogPrint` has many issues:
* It seems to indicate that something is being "printed", however config options such as `-printtoconsole` actually control what and where something is logged.
* It does not mention the log severity (debug).
* It is a deprecated alias for `LogDebug`, according to the dev notes.
* It wastes review cycles, because reviewers sometimes point out that it is deprecated.
* It makes the code inconsistent, when both are used, possibly even in lines right next to each other (like in `InitHTTPServer`)
Fix all issues by removing the deprecated alias.
I checked all conflicting pull requests and at the time of writing there are no conflicts, except in pull requests that are marked as draft, are yet unreviewed, or are blocked on feedback for other reasons. So I think it is fine to do now.
ACKs for top commit:
stickies-v:
ACK fa09cb41f5
danielabrozzoni:
utACK fa09cb41f5
TheCharlatan:
ACK fa09cb41f5
Tree-SHA512: 14270f4cfa3906025a0b994cbb5b2e3c8c2427c0beb19c717a505a2ccbfb1fd1ecf2fd03f6c52d22cde69a8d057e50d2207119fab2c2bc8228db3f10d4288d0f
faa382ae76 ci, doc: Drop reference to `src/.bear-tidy-config` (Hennadii Stepanov)
d71ac76842 build: Remove Autotools-based build system (Hennadii Stepanov)
e268b48419 doc: Adjust `doc/design/libraries.md` (Hennadii Stepanov)
d209e4f156 doc: Drop mentions of `share/genbuild.sh` (Hennadii Stepanov)
Pull request description:
This PR deletes the Autotools-based build system.
The MSVC build system is deleted in https://github.com/bitcoin/bitcoin/pull/30731.
ACKs for top commit:
maflcko:
re-ACK faa382ae76🍦
TheCharlatan:
ACK faa382ae76
fanquake:
ACK faa382ae76
Tree-SHA512: 53df977b5b199a1c38f7f61a042a62b24831c559ba65a461b4ac1c96a1a56e2dfd676df79f1358fd1cc1749ff27e7b548086157f337d4f596c1054cb3d2d5739
bd7ce05f9d test: fix `TestShell` initialization (late follow-up for #30463) (Sebastian Falbesoner)
Pull request description:
Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master:
```
$ python3
Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, setup_clean_chain=True)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/thestack/bitcoin/test/functional/test_framework/test_shell.py", line 70, in __new__
TestShell.instance = TestShell.__TestShell()
TypeError: BitcoinTestFramework.__init__() missing 1 required positional argument: 'test_file'
```
Since #30463, BitcoinTestFramework instances expect the path of the calling test at construction, in order to find shared data like the configuration (config.ini) and the cache. Note that in contrast to actual functional tests, we can't simply pass `__file__` here, as the test shell module sits within the `test_framework` subfolder, so we have to navigate up to the parent directory and append some dummy test file name.
On the long-term we should probably add some TestShell instantation smoke-test to detect issues like this early. As I'm not too familiar with the CI I'm not sure what is a good way to achieve this (a functional test obviously can't be used, as that's already a BitcoinTestFramework test in itself), but happy to take suggestions.
ACKs for top commit:
ismaelsadeeq:
Tested ACK bd7ce05f9d
danielabrozzoni:
tACK bd7ce05f9d
brunoerg:
ACK bd7ce05f9d
Tree-SHA512: c3a2365e2cda48a233ee724673c490787981354914f33e10eadbbad9c68e8403d84c5551229a611401e743886539de380ba4bfcb77032b6c85731e3bbe962dc1
a563f41232 Remove second node since only 1 is needed for the test (Martin Saposnic)
1f4cdb3d69 Replace custom funding tx creation with MiniWallet. (Martin Saposnic)
Pull request description:
In response to issue https://github.com/bitcoin/bitcoin/issues/30600, optimizations have been implemented to enhance test efficiency and readability:
This PR refactors the `rpc_signrawtransactionwithkey.py` functional test to use MiniWallet for creating funding transactions. This simplifies the test code and improves performance by eliminating the need to mine new blocks for each funding transaction.
Key changes:
- Replaced custom `send_to_address` method with MiniWallet's `send_to` method
- Removed unnecessary setup of a clean chain and second node
- Simplified transaction creation and signing process
ACKs for top commit:
glozow:
ACK a563f41232
ismaelsadeeq:
code review ACK a563f41232
theStack:
ACK a563f41232
Tree-SHA512: 318959f89702b169453d537dafb822f5ef1921db1088941d8bbdb3171dd7a6ecad590e57a3802bc37bcf8992267ed6ffa7f156b229d9817ebf812bd35df509b5
