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
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.
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
