The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
a24e633339 refactor: rpc: set TxToJSON default verbosity to SHOW_DETAILS (stickies-v)
Pull request description:
`TxToJSON()` and `TxToUniv()` are only to be called when we want to decode the transaction (i.e. its details) into JSON. If `TxVerbosity` is `SHOW_TXID`, the function should not have been (and currently is not) called in the first place.
There is no behaviour change, current logic simply assumes anything less than `TxVerbosity::SHOW_DETAILS_AND_PREVOUT` equals `TxVerbosity::SHOW_DETAILS`. With this change, the assumptions and intent become more explicit.
ACKs for top commit:
w0xlt:
ACK a24e633339
Tree-SHA512: b97235adae49b972bdbe10aca1438643fb35ec66a4e57166b1975b3015bc5a06a711feebe4453a8fefe71781e484b21ef80847d8e8a33694a3abcc863accd4d7
faf7b4f1fc Add BlockManager::IsPruneMode() (MarcoFalke)
fae71fe27e Add BlockManager::GetPruneTarget() (MarcoFalke)
fa0f0436d8 Add BlockManager::LoadingBlocks() (MarcoFalke)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1061323795, but adding getters seems unrelated from removing globals, so I split it out for now.
ACKs for top commit:
dergoegge:
Code review ACK faf7b4f1fc
brunoerg:
crACK faf7b4f1fc
Tree-SHA512: 204d0e9a0e8b78175482f89b4ce620fba0e65d8e49ad845d187af44d3843f4c733a01bac1ffe5a5319f524d8346123693a456778b69d6c75268c447eb8839642
`TxToJSON()` and `TxToUniv()` are only to be called when we want to
decode the transaction (i.e. its details) into JSON. If `TxVerbosity` is
`SHOW_TXID`, the function should not have been (and currently is not)
called in the first place.
There is no behaviour change, current logic simply assumes anything less
than `TxVerbosity::SHOW_DETAILS_AND_PREVOUT` equals
`TxVerbosity::SHOW_DETAILS`. With this change, the assumptions and
intent become more explicit.
fa035fe2d6 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cda Use DataStream where possible (MarcoFalke)
fa9becfe1c streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](fa035fe2d6)
aureleoules:
diff re-ACK fa035fe2d6fa0e6640ba..fa035fe2d6
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
fafeddfe0e rpc: Throw more user friendly arg type check error (MarcoFalke)
Pull request description:
The arg type check error doesn't list which arg (position or name) failed. Fix that.
ACKs for top commit:
stickies-v:
ACK fafeddfe0e - although I think the functional test isn't in a logical place (but not blocking)
Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
-BEGIN VERIFY SCRIPT-
sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.h src/rpc/util.cpp
sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
-END VERIFY SCRIPT-
3d1a4d8a45 RPC: make RPCResult::MatchesType return useful errors (Anthony Towns)
Pull request description:
Currently if you don't correctly update the description of the return value for an RPC call, you essentially just get an assertion failure with no useful information; this generates a description of the problems instead.
ACKs for top commit:
MarcoFalke:
re-ACK 3d1a4d8a45🌷
Tree-SHA512: cf0580b7046faab0128672a74f8cc5a1655dfdca6646a2e38b51f0fb5f672c98aad6cd4c5769454a2d644a67da639ccb1c8ff5d24d3d6b4446a082398a643722
fad56f7dd6 doc: Properly report optional RPC args (MarcoFalke)
fa09cb6086 refactor: Introduce is_top_level_arg (MarcoFalke)
fab92a5a5a refactor: Remove const to fix performance-move-const-arg clang-tidy errors (MarcoFalke)
Pull request description:
`OMITTED_NAMED_ARG` and `OMITTED` are a confusing burden:
* It puts the burden on developers to pick the right one of the two
* They can be interchanged without introducing a compile failure or other error
* Picking the wrong one is leading to incorrect docs
* They are redundant, because the correct one can already be determined by the surrounding type
Fix all issues by making them an alias of each other; Pick the right one based on the outer type.
ACKs for top commit:
fanquake:
ACK fad56f7dd6
Tree-SHA512: 6e7193a05a852ba1618a9cb3261220c7ad3282bc5595325c04437aa811f529a88e2851e9c7dbf9878389b8aa42e98f8817b7eb0260fbb9e123da0893cbae6ca2
6d0ab07e81 refactor: use convenience fn to auto parse non-string parameters (stickies-v)
Pull request description:
Minimizes code duplication and improves function naming by having a single (overloaded) convenience function `ParseIfNonString` that both checks if the parameter is a non-string parameter and automatically parses the value if so.
ACKs for top commit:
aureleoules:
ACK 6d0ab07e81
Tree-SHA512: 8cbf68a17cfbdce1e31a19916f447a2965c139fdef00c19e32a9b679f4a4015dfe69ceea0bbe1723711e1c5033ea8d4005d1f4485dfbeea22226140f8cbe8aa3
1dc0e4bc6f rpc: remove optional from fStateStats fields (fanquake)
Pull request description:
These are no-longer optional after #26515, so remove the documentation, and no-op `fStateStats` checks.
ACKs for top commit:
dergoegge:
Code review ACK 1dc0e4bc6f
Tree-SHA512: 06d4550e866341b379bfdbc72d67d71a3b7ceceec06ebd4c5e6f178b75fe40cbf4aff51adba1bc52590e69e818cbdecb0366bf1528c59c5c3dff5bbdba8eac68
The warnings look like:
src/rpc/util.h:192:19: error: std::move of the const variable 'name' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg,-warnings-as-errors]
: m_names{std::move(name)},
^~~~~~~~~~ ~
fa9f6d7bcd rpc: Run type check against RPCArgs (MarcoFalke)
faf96721a6 test: Fix wrong types passed to RPCs (MarcoFalke)
Pull request description:
It seems brittle to require `RPCTypeCheck` being called inside the code logic. Without compile-time enforcement this will lead to places where it is forgotten and thus to inconsistencies and bugs. Fix this by removing the calls to `RPCTypeCheck` and doing the check internally.
The changes should be reviewed as refactoring changes. However, if they change behavior, it will be a bugfix. For example the changes here happen to also detect/fix bugs like the one fixed in commit 3b5fb6e77a.
ACKs for top commit:
ajtowns:
ACK fa9f6d7bcd
Tree-SHA512: fb2c0981fe6e24da3ca7dbc06898730779ea4e02ea485458505a281cf421015e44dad0221a04023fc547ea2c660d94657909843fc85d92b847611ec097532439
5ca7a7be76 rpc: Return accurate results for scanblocks (Aurèle Oulès)
Pull request description:
Implements #26322.
Adds a `filter_false_positives` mode to `scanblocks` to accurately verify results from blockfilters.
If the option is enabled, pre-results given by blockfilters will be filtered out again by checking vouts and vins of all transactions of the relevant blocks against the given descriptors.
### Master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]'
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (without `filter_false_positives` mode)
Same as master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=false
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (with `filter_false_positives` mode)
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=true
{
"from_height": 0,
"to_height": 2376058,
"relevant_blocks": [
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
Also adds a test to check that the blockhash of a transaction will be included in the `relevant_blocks` whether the `filter_false_positives` mode is enabled or not.
ACKs for top commit:
achow101:
ACK 5ca7a7be76
theStack:
re-ACK 5ca7a7be76
furszy:
Code review ACK 5ca7a7be
Tree-SHA512: e8f3cceddddd66f59509717b6314d89e2fef241e13cee81b18fd95e8362cbb95cc40f884342ce6cf892a86febd9e2d434afce05d51892240e67f72ae991852e7
282019cd3d refactor: add kernel/cs_main.* (fanquake)
Pull request description:
One place to find / include `cs_main`.
No more:
> // Actually declared in validation.cpp; can't include because of circular dependency.
> extern RecursiveMutex cs_main;
Ultimately, no more need to include `validation.h` (which also includes (heavy/boost filled) `txmempool.h`) everywhere for `cs_main`. See #26087 for another example of why that is useful.
ACKs for top commit:
ajtowns:
ACK 282019cd3d
Tree-SHA512: 142835b794873e7a09c3246d6101843ae81ec0c6295e6873130c98a2abfa5f7282748d0f1a37237a779cc71c3bc0a75d03b20313ef5398c83d4814215cbc8287
264f9ef17f [validation] return MempoolAcceptResult for every tx on PCKG_TX failure (glozow)
dae81e01e8 [refactor] rename variables in AcceptPackage for clarity (glozow)
da484bc738 [doc] release note effective-feerate and effective-includes RPC results (glozow)
5eab397b98 [validation] remove PackageMempoolAcceptResult::m_package_feerate (glozow)
601bac88cb [rpc] return effective-includes in testmempoolaccept and submitpackage (glozow)
1691eaa818 [rpc] return effective-feerate in testmempoolaccept and submitpackage (glozow)
d6c7b78ef2 [validation] return wtxids of other transactions whose fees were used (glozow)
1605886380 [validation] return effective feerate from mempool validation (glozow)
5d35b4a7de [test] package validation quits early due to non-policy, non-missing-inputs failure (glozow)
be2e4d94e5 [validation] when quitting early in AcceptPackage, set package_state and tx result (glozow)
Pull request description:
This PR fixes a bug and improves the mempool accept interface to return information more predictably.
Bug: In package validation, we first try the transactions individually (see doc/policy/packages.md for more explanation) and, if they all failed for missing inputs and policy-related (i.e. fee) reasons, we'll try package validation. Otherwise, we'll just "quit early" since, for example, if a transaction had an invalid signature, adding a child will not help make it valid. Currently, when we quit early, we're not setting the `package_state` to be invalid, so the caller might think it succeeded. Also, we're returning no results - it makes more sense to return the individual transaction failure. Thanks instagibbs for catching https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1013293248!
Also, make the package results interface generally more useful/predictable:
- Always return the feerate at which a transaction was considered for `CheckFeeRate` in `MempoolAcceptResult::m_effective_feerate` when it was successful. This can replace the current `PackageMempoolAcceptResult::m_package_feerate`, which only sometimes exists.
- Always provide an entry for every transaction in `PackageMempoolAcceptResult::m_tx_results` when the error is `PCKG_TX`.
ACKs for top commit:
instagibbs:
reACK 264f9ef17f
achow101:
ACK 264f9ef17f
naumenkogs:
reACK 264f9ef17f
Tree-SHA512: ce7fd9927a80030317cc6157822596e85a540feff5dbf5eea7c62da2eb50c917cdddc9da1e2ff62cc18b98b27d360151811546bd9d498859679a04bbee090837
This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.
Minimizes code duplication and improves function naming by having
a single (overloaded) convenience function that both checks if
the parameter is a non-string parameter and automatically parses the
value if so.
47c4b1f52a mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f739 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)
Pull request description:
Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.
## Unexpected behaviour
This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.
In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.
## Improvements
### Return value instead of out-parameters
This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.
### Observability
There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.
ACKs for top commit:
achow101:
ACK 47c4b1f52a
w0xlt:
ACK 47c4b1f52a
glozow:
ACK 47c4b1f52a
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52a
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
6fefd49527 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande)
Pull request description:
The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer.
Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in https://github.com/bitcoin/bitcoin/pull/26457#pullrequestreview-1181641835).
But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data.
An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see 5f900e27d0). Since this conflicts with #25923 and should be a separate discussion, I removed that commit from this PR.
ACKs for top commit:
dergoegge:
Approach ACK 6fefd49527
MarcoFalke:
review ACK 6fefd49527👒
Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532
8c3ff7d52a test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563825 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8c rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e67 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52a 🗂
kristapsk:
ACK 8c3ff7d52a
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
This makes the code more robust, see previous commit.
In general replacing isTrue with get_bool is not equivalent because
get_bool can throw exceptions, but in this case, exceptions won't happen
because of RPCTypeCheck() and isNull() checks in the preceding code.
