17845e7f21 rpc: add utxo's blockhash and number of confirmations to scantxoutset output (Luis Schwab)
Pull request description:
This PR resolves#30478 by adding two fields to the `scantxoutset` RPC:
- blockhash: the blockhash that an UTXO was created
- confirmations: the number of confirmations an UTXO has relative to the chaintip.
The rationale for the first field is that a blockhash is a much more reliable identifier than the height:
> When using the scantxoutset RPC, the current behaviour is to show the block height of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query. In this case, an UTXO that does not exist would be assumed to exist, unless the chain's tip hash is recorded before the scan, and make sure it still exists after, as per https://github.com/bitcoindevkit/bdk/issues/895#issuecomment-1475766797 comment by evanlinjin.
The second one was suggested by maflcko, and I agree it's useful for human users:
> While touching this, another thing to add could be the number of confirmations? I understand that this wouldn't help machine consumers of the interface, but human callers may find it useful?
This will yield an RPC output like so:
```diff
bitcoin-cli scantxoutset start "[\"addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)\"]"
{
"success": true,
"txouts": 185259116,
"height": 853622,
"bestblock": "00000000000000000002e97d9be8f0ddf31829cf873061b938c10b0f80f708b2",
"unspents": [
{
"txid": "fae435084345fe26e464994aebc6544875bca0b897bf4ce52a65901ae28ace92",
"vout": 0,
"scriptPubKey": "0014a00b1ad58d24ad8433c56662bfb45596cd5fa7d6",
"desc": "addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)#smk4xmt7",
"amount": 0.00091190,
"coinbase": false,
"height": 852741,
+ "blockhash": "00000000000000000002eefe7e7db44d5619c3dace4c65f3fdcd2913d4945c13",
+ "confirmations": 882
}
],
"total_amount": 0.00091190
}
```
ACKs for top commit:
sipa:
utACK 17845e7f21
Eunovo:
ACK 17845e7f21
tdb3:
ACK 17845e7f21
Tree-SHA512: 02366d0004e5d547522115ef0efe6794a35978db53dda12c675cfae38197bf43f0bf89ca99a3d79e3d2cff95186015fe1ab764abb8ab82bda440ae9302ad973b
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
b4dd7ab43e logging: use std::string_view (Anthony Towns)
558df5c733 logging: Apply formatting to early log messages (Anthony Towns)
6cf9b34440 logging: Limit early logging buffer (Anthony Towns)
0b1960f1b2 logging: Add DisableLogging() (Anthony Towns)
6bbc2dd6c5 logging: Add thread safety annotations (Anthony Towns)
Pull request description:
In order to cope gracefully with `Log*()` calls that are invoked prior to logging being fully configured (indicated by calling `StartLogging()` we buffer early log messages in `m_msgs_before_open`. This has a couple of minor issues:
* if there are many such log messages the buffer can become arbitrarily large; this can be a problem for users of libkernel that might not wish to worry about logging at all, and as a result never invoke `StartLogging()`
* early log messages are formatted before the formatting options are configured, leading to inconsistent output
Fix those issues by buffering the log info prior to formatting it, and setting a limit on the size of the buffer (dropping the oldest lines, and reporting the number of lines skipped).
Also adds some thread safety annotations, and the ability to invoke `LogInstance().DisableLogging()` if you want to disable logging entirely, for a minor efficiency improvement.
ACKs for top commit:
maflcko:
re-ACK b4dd7ab43e 🕴
ryanofsky:
Code review ACK b4dd7ab43e
TheCharlatan:
Nice, ACK b4dd7ab43e
Tree-SHA512: 966660181276939225a9f776de6ee0665e44577d2ee9cc76b06c8937297217482e6e426bdc5772d1ce533a0ba093a8556b6a50857d4c876ad8923e432a200440
fae0db0360 fuzz: Deglobalize signature cache in sigcache test (TheCharlatan)
Pull request description:
The body of the fuzz test should ideally be a pure function. If data is persisted in the cache over many iterations, and there is a crash, reproducing it from the input might be difficult. Solve this by getting rid of the global state. This is a follow-up from #30425.
ACKs for top commit:
dergoegge:
utACK fae0db0360
ryanofsky:
Code review ACK fae0db0360
Tree-SHA512: 93dcbb9f2497f13856970469042d6870f04de10fe206827a8db1aae7fc8f3ac7fd900bee7945b5fe4c9e33883268dabb15be7e7bc91cf353ffc0d118cd60e97d
647fa37cdb bench: add cluster linearization improvement benchmark (Pieter Wuille)
28549791b3 clusterlin: permit passing in existing linearization to Linearize (Pieter Wuille)
97d98718b0 clusterlin: add LinearizationChunking class (Pieter Wuille)
d5918dc3c6 clusterlin: randomize the SearchCandidateFinder search order (Pieter Wuille)
991ff9a9a4 clusterlin: use bounded BFS exploration (optimization) (Pieter Wuille)
d9b235e7d2 bench: Candidate finding and linearization benchmarks (Pieter Wuille)
46aad9b099 clusterlin: add Linearize function (Pieter Wuille)
ee0ddfe4f6 clusterlin: add chunking algorithm (Pieter Wuille)
2a41f151af clusterlin: add SearchCandidateFinder class (Pieter Wuille)
4828079db3 clusterlin: add AncestorCandidateFinder class (Pieter Wuille)
58f7e01db4 tests: framework for testing DepGraph class (Pieter Wuille)
a6e07e769a clusterlin: introduce cluster_linearize.h with Cluster and DepGraph types (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289
This introduces low-level cluster linearization code, including tests and some benchmarks. It is currently not hooked up to anything.
Ultimately, what this PR adds is a function `Linearize` which operates on instances of `DepGraph` (instances of which represent pre-processed transaction clusters) to produce and/or improve linearizations for that cluster.
To provide assurance, the code heavily relies on fuzz tests. A novel approach is used here, where the fuzz input is parsed using the serialization.h framework rather than `FuzzedDataProvider`, with a custom serializer/deserializer for `DepGraph` objects. By including serialization, it's possible to ascertain that the format can represent every relevant cluster, as well as potentially permitting the construction of ad-hoc fuzz inputs from clusters (not included in this PR, but used during development).
---
The `Linearize(depgraph, iteration_limit, rng_seed, old_linearization)` function is an implementation of the (single) LIMO algorithm, with the $S$ in every iteration found as the best out of (a) the best remaining ancestor set and (b) randomized computationally-bounded search. It incrementally builds up a linearization by finding good topologically-valid subsets to move to the front, in such a way that the resulting linearization has a diagram that is at least as good as the `old_linearization` passed in (if any).
* Despite using both best ancestor set and search, this is not Double LIMO, as no intersections between these are involved; just the best of the two.
* The `iteration_limit` and `rng_seed` only control the (b) randomized search. Even with 0 iterations, the result will be as good as the old linearization, and the included sets at every point will have a feerate at least as high as the best remaining ancestor set at that point.
The search algorithm used in the (b) step is very basic, and largely matches Section 2.1 of [How to Linearize your Cluster.](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-21-searching-6). See #30286 for optimizations to make it more efficient.
For background and references, see [Introduction to cluster linearization](https://delvingbitcoin.org/t/introduction-to-cluster-linearization/1032).
ACKs for top commit:
instagibbs:
reACK 647fa37cdb
glozow:
reACK 647fa37cdb, both code and mermaid diagram look correct to me
sdaftuar:
ACK 647fa37cdb
Tree-SHA512: 52c8aa3d1d91190bf1265a947d2712e9d12f745313ffceef6ae7e3ff517d01d8b3b9b4ce6066298d59751c4ba90555a3c0171229868ba50100f588a2aa6a486d
7aa8994c6f refactor: Add FlatFileSeq member variables in BlockManager (TheCharlatan)
Pull request description:
Instead of constructing a new class every time a file operation is done, construct them once for each of the undo and block file when a new BlockManager is created.
In future, this might make it easier to introduce an abstract block store.
Historically, this was not easily possible prior to #27125.
ACKs for top commit:
danielabrozzoni:
ACK 7aa8994c6f
tdb3:
ACK 7aa8994c6f
stickies-v:
ACK 7aa8994c6f
brunoerg:
utACK 7aa8994c6f
Tree-SHA512: 7c181968c270956c90fa0f3687562239912a973b6a35ddbf49fc58733247ea9d986303cbf6f8fc16e8c2d9bf4505e866aed37f030a8c9be72e95bf3752902aa6
c399c80a09 cleanse: Use SecureZeroMemory for mingw-w64 (release) builds (fanquake)
Pull request description:
This PR switches our Windows release builds to use the [`SecureZeroMemory()`](https://learn.microsoft.com/en-us/previous-versions/windows/desktop/legacy/aa366877(v=vs.85)) provided by mingw-w64.
ACKs for top commit:
sipa:
utACK c399c80a09
TheCharlatan:
ACK c399c80a09
Tree-SHA512: dbb20b16c85061d2f9408a3cf69cecc16765f8f61b25a1707146767b664c7ad0caf36975380814ef8e7c49a30199daebac6d5d7a3585354d1adac8e9770199c6
It encapsulates a given linearization in chunked form, permitting arbitrary
subsets of transactions to be removed from the linearization. Its purpose
is adding the Intersect function, which is a crucial operation that will
be used in a further commit to make Linearize improve existing linearizations.
Switch to BFS exploration of the search tree in SearchCandidateFinder
instead of DFS exploration. This appears to behave better for real
world clusters.
As BFS has the downside of needing far larger search queues, switch
back to DFS temporarily when the queue grows too large.
Add benchmarks for known bad graphs for the purpose of search (as
an upper bound on work per search iterations) and ancestor sorting
(as an upper bound on linearization work with no search iterations).
This adds a first version of the overall linearization interface, which given
a DepGraph constructs a good linearization, by incrementally including good
candidate sets (found using AncestorCandidateFinder and SearchCandidateFinder).
This introduces a bespoke fuzzing-focused serialization format for DepGraphs,
and then tests that this format can represent any graph, roundtrips, and then
uses that to test the correctness of DepGraph itself.
This forms the basis for future fuzz tests that need to work with interesting
graphs.
This primarily adds the DepGraph class, which encapsulates precomputed
ancestor/descendant information for a given transaction cluster, with a
number of utility features (inspectors for set feerates, computing
reduced parents/children, adding transactions, adding dependencies), which
will become needed in future commits.
f46b220256 fuzz: Use BasicTestingSetup for coins_view target (TheCharlatan)
9e2a723d5d test: Add arguments for creating a slimmer setup (TheCharlatan)
Pull request description:
This adds arguments to some of the testing setup constructors for creating an environment without networking and a validation interface. This is useful for improving the performance of the utxo snapshot fuzz test, which constructs a new TestingSetup on each iteration.
Using this slimmed down `TestingSetup` in future might also make the tests a bit faster when run in aggregate.
ACKs for top commit:
maflcko:
review ACK f46b220256
dergoegge:
utACK f46b220256
Tree-SHA512: 9dc62512b127b781fc9e2d8ef2b5a9b06ebb927a8294b6d872001c553984a7eb1f348e0257b32435b34b5505b5d0323f73bdd572a673da272d3e1e8538ab49d6
fac0c3d4bf doc: Add release notes for two pull requests (MarcoFalke)
fa7b57e5f5 refactor: Replace ParseHashStr with FromHex (MarcoFalke)
fa90777245 rest: Reject truncated hex txid early in getutxos parsing (MarcoFalke)
fab6ddbee6 refactor: Expose FromHex in transaction_identifier (MarcoFalke)
fad2991ba0 refactor: Implement strict uint256::FromHex() (MarcoFalke)
fa103db2bb scripted-diff: Rename SetHex to SetHexDeprecated (MarcoFalke)
fafe4b8051 test: refactor: Replace SetHex with uint256 constructor directly (MarcoFalke)
Pull request description:
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
ACKs for top commit:
stickies-v:
re-ACK fac0c3d4bf - only doc and test updates to address review comments, thanks!
hodlinator:
ACK fac0c3d4bf
Tree-SHA512: 473feb3fcf6118443435d1dd321006135b0b54689bfbbcb1697bb5811a449bef51f475c715de6911ff3c4ea3bdb75f601861ff93347bc4414d6b9e5298105dd7
25bf86a225 [test]: ensure `estimatesmartfee` default mode is `economical` (ismaelsadeeq)
41a2545046 [fees]: change `estimatesmartfee` default mode to `economical` (ismaelsadeeq)
Pull request description:
Fixes#30009
This PR changes the `estimatesmartfee` default mode to `economical`.
This was also suggested on IRC https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-04-26#1021609
- `conservative` mode: This is the `estimatesmartfee` RPC mode which considers a longer history of blocks. It potentially returns a higher fee rate and is more likely to be sufficient for the desired target, but it is not as responsive to short-term drops in the prevailing fee market.
- `economical` mode: This is the `estimatesmartfee` RPC mode where estimates are potentially lower and more responsive to short-term drops in the prevailing fee market.
Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the `conservative` mode overestimates, see
https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.
ACKs for top commit:
instagibbs:
reACK 25bf86a225
glozow:
ACK 25bf86a225
willcl-ark:
ACK 25bf86a225
Tree-SHA512: 78ebda667eb9c8f87dcc2f0e6c14968bd1de30358dc77a13611b186fb8427ad97d9f537bad6e32e0a1aa477ccd8c64fee4d41e19308ef3cb184ff1664e6ba8a6
This is a safe replacement of the previous SetHex, which now returns an
optional to indicate success or failure.
The code is similar to the ParseHashStr helper, which will be removed in
a later commit.
These cause compile failures with _LIBCPP_REMOVE_TRANSITIVE_INCLUDES.
i.e:
```bash
In file included from init.cpp:8:
./init.h:46:54: error: no template named 'atomic' in namespace 'std'
46 | bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status);
| ~~~~~^
1 error generated.
```
See: https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html.
Now that m_txrequest and m_recent_confirmed_transactions are guarded by
the same mutex, there is no benefit to processing them separately.
Instead, just loop through pblock->vtx once.
c85accecaf [refactor] delete EraseTxNoLock, just use EraseTx (glozow)
6ff84069a5 remove obsoleted TxOrphanage::m_mutex (glozow)
61745c7451 lock m_recent_confirmed_transactions using m_tx_download_mutex (glozow)
723ea0f9a5 remove obsoleted hashRecentRejectsChainTip (glozow)
18a4355250 update recent_rejects filters on ActiveTipChange (glozow)
36f170d879 add ValidationInterface::ActiveTipChange (glozow)
3eb1307df0 guard TxRequest and rejection caches with new mutex (glozow)
Pull request description:
See #27463 for full project tracking.
This contains the first few commits of #30110, which require some thinking about thread safety in review.
- Introduce a new `m_tx_download_mutex` which guards the transaction download data structures including `m_txrequest`, the rolling bloom filters, and `m_orphanage`. Later this should become the mutex guarding `TxDownloadManager`.
- `m_txrequest` doesn't need to be guarded using `cs_main` anymore
- `m_recent_confirmed_transactions` doesn't need its own lock anymore
- `m_orphanage` doesn't need its own lock anymore
- Adds a new `ValidationInterface` event, `ActiveTipChanged`, which is a synchronous callback whenever the tip of the active chainstate changes.
- Flush `m_recent_rejects` and `m_recent_rejects_reconsiderable` on `ActiveTipChanged` just once instead of checking the tip every time `AlreadyHaveTx` is called. This should speed up calls to that function (no longer comparing a block hash each time) and removes the need to lock `cs_main` every time it is called.
Motivation:
- These data structures need synchronization. While we are holding `m_tx_download_mutex`, these should hold:
- a tx hash in `m_txrequest` is not also in `m_orphanage`
- a tx hash in `m_txrequest` is not also in `m_recent_rejects` or `m_recent_confirmed_transactions`
- In the future, orphan resolution tracking should also be synchronized. If a tx has an entry in the orphan resolution tracker, it is also in `m_orphanage`, and not in `m_txrequest`, etc.
- Currently, `cs_main` is used to e.g. sync accesses to `m_txrequest`. We should not broaden the scope of things it locks.
- Currently, we need to know the current chainstate every time we call `AlreadyHaveTx` so we can decide whether we should update it. Every call compares the current tip hash with `hashRecentRejectsChainTip`. It is more efficient to have a validation interface callback that updates the rejection filters whenever the chain tip changes.
ACKs for top commit:
instagibbs:
reACK c85accecaf
dergoegge:
Code review ACK c85accecaf
theStack:
Light code-review ACK c85accecaf
hebasto:
ACK c85accecaf, I have reviewed the code and it looks OK.
Tree-SHA512: c3bd524b5de1cafc9a10770dadb484cc479d6d4c687d80dd0f176d339fd95f73b85cb44cb3b6b464d38a52e20feda00aa2a1da5a73339e31831687e4bd0aa0c5
Instead of constructing a new class every time a file operation is done,
construct them once for each of the undo and block file when a new
BlockManager is created.
In future, this might make it easier to introduce an abstract block
store.
SetHex is fragile, because it accepts any non-hex input or any length of
input, without error feedback. This can lead to issues when the input is
truncated or otherwise corrupted.
Document the problem by renaming the method.
In the future, the fragile method should be removed from the public
interface.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetHex/SetHexDeprecated/g' $( git grep -l SetHex ./src )
-END VERIFY SCRIPT-
09ce3501fa fix: Make TxidFromString() respect string_view length (Hodlinator)
01e314ce0a refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator)
2f5577dc2e test: uint256 - Garbage suffixes and zero padding (Hodlinator)
f11f816800 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator)
f0eeee2dc1 test: Add test for TxidFromString() behavior (Ryan Ofsky)
Pull request description:
### Problem
Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`.
### Solution
Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in.
(PR was prompted by comment in https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200 (referring to https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378)).
ACKs for top commit:
maflcko:
re-ACK 09ce3501fa🕓
paplorinc:
ACK 09ce3501fa
ryanofsky:
Code review ACK 09ce3501fa. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.
Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581
29eafd5733 rpc: doc: use "output script" terminology consistently in "asm"/"hex" results (Sebastian Falbesoner)
Pull request description:
The wording "public key script" was likely chosen as a human-readable form of the technical term `scriptPubKey`, but it doesn't seem to be really widespread. Replace it by the more (probably most?) common term "output script" instead. Note that the argument for the `decodescript` RPC is not necessarily an output script (it could e.g. be also a redeem script), so in this case we just stay generic and use "script".
See also the draft BIP "Terminology for Transaction Components" (https://github.com/murchandamus/bips/blob/2022-04-tx-terminology/bip-tx-terminology.mediawiki) from murchandamus which suggests to use "output script" as well.
Affects the help text of the following RPCs:
- decodepsbt
- decoderawtransaction
- decodescript
- getblock (if verbosity=3)
- getrawtransaction (if verbosity=2,3)
- gettxout
ACKs for top commit:
maflcko:
ACK 29eafd5733
achow101:
ACK 29eafd5733
BrandonOdiwuor:
ACK 29eafd5733
tdb3:
ACK 29eafd5733
Tree-SHA512: 62eb92d42bc44e36dc3090df7b248a123868a74af253d2046de02086e688bf6ff98307b927ba2fee3d599f85e073aeb8eca90ed15105ca63b648b6796cfa340b
Otherwise, the debug log could read confusingly, when the getpeerinfo()
RPC (calling GetNodeStats) happens after the "accepted connection" log
line, but returns an empty list.
For example, the following timeline in the debug log could correspond to
a getpeerinfo reply that is empty:
[net] [net.cpp:3764] [CNode] Added connection peer=0
[net] [net.cpp:1814] [CreateNodeFromAcceptedSocket] connection from 127.0.0.1:45154 accepted
[http] [httpserver.cpp:305] [http_request_cb] Received a POST request for / from 127.0.0.1:33320
[httpworker.1] [rpc/request.cpp:232] [parse] ThreadRPCServer method=getpeerinfo user=__cookie__
Fix it by moving the log line.
Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378.
