This is preparation for the simulation fuzz test added in a later commit. Since
AddChildrenToWorkSet consumes randomness, there is no way for the simulator to
exactly predict its behavior. By returning the set of made-reconsiderable announcements
instead, the simulator can instead test that it is *a* valid choice, and then
apply it to its own data structures.
For the default number of peers (125), allows each to relay a default
descendant package (up to 25-1=24 can be missing inputs) of small (9
inputs or fewer) transactions out of order.
This limit also gives acceptable bounds for worst case LimitOrphans iterations.
Functional tests aren't changed to check for larger cap because it would
make the runtime too long.
Also deletes the now-unused DEFAULT_MAX_ORPHAN_TRANSACTIONS.
This is largely a reimplementation using boost::multi_index_container.
All the same public methods are available. It has an index by outpoint,
per-peer tracking, peer worksets, etc.
A few differences:
- Limits have changed: instead of a global limit of 100 unique orphans,
we have a maximum number of announcements (which can include duplicate
orphans) and a global memory limit which scales with the number of
peers.
- The maximum announcements limit is 100 to match the original limit,
but this is actually a stricter limit because the announcement count
is not de-duplicated.
- Eviction strategy: when global limits are reached, a per-peer limit
comes into play. While limits are exceeded, we choose the peer whose
“DoS score” (max usage / limit ratio for announcements and memory
limits) is highest and evict announcements by entry time, sorting
non-reconsiderable ones before reconsiderable ones. Since announcements
are unique by (wtxid, peer), as long as 1 announcement remains for a
transaction, it remains in the orphanage.
- This eviction strategy means no peer can influence the eviction of
another peer’s orphans.
- Also, since global limits are a multiple of per-peer limits, as long
as a peer does not exceed its limits, its orphans are protected from
eviction.
- Orphans no longer expire, since older announcements are generally
removed before newer ones.
- GetChildrenFromSamePeer returns the transactions from newest to
oldest.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Move towards a model where TxOrphanage is initialized with limits that
it remembers throughout its lifetime.
Remove the param. Limiting by number of unique orphans will be removed
in a later commit.
Now that -maxorphantx is gone, this does not change the node behavior.
The parameter is only used in tests.
Expiry is going away in a later commit.
This is only an RPC change. Behavior of the orphanage does not change.
Note that getorphantxs is marked experimental.
a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using
`std::variant` we have no portable zero-overhead way of accessing them,
so use `std::visit` and drop `bool wtxid` in-parameter.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
fcfd3db563e89fd79820a4cdfa102d624d801de1 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795fd3bff79d790102ca7cefa8fd9b204c use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
achow101:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
furszy:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea4b578922a3316b37b486f96cb0beec util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df9ad45faf25c32c99a4dd70759b25be Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b273b6db5d2212ba91cfc713c1634c5d rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
achow101:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
hodlinator:
re-ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
9341b5333ad54ccdb7c16802ff06c51b956948e7 blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee0b108ebbb8afedc47d73ce0f97d272 test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6dd0cc23175a1484e8ebb6cdc6e2183 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7 🕙
achow101:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
hodlinator:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
janb84:
re ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6 node: cap -dbcache to 1GiB on 32-bit architectures (Antoine Poinsot)
2c43b6adebbfabb3c8dd82fe821ce0a5d6173b3b init: cap -maxmempool to 500 MB on 32-bit systems (Antoine Poinsot)
Pull request description:
32-bit architecture is limited to 4GiB of RAM, so it doesn't make sense to set a too high value. A too high value could cause an OOM unbeknownst to the user a while after startup as mempool / dbcache fills.
ACKs for top commit:
achow101:
ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
instagibbs:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
dergoegge:
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
glozow:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
Tree-SHA512: cc7541b2c0040fc21a43916caec464dfb443af808f4e85deffa1187448ffff6edb0d69f9ebdb43915d145b8b4694d8465afe548f88da53ccebc9ce4b7c34b735
a18e57232867d946bc35769632fed49e1bf1464f test: more template verification tests (Sjors Provoost)
10c908808fb80cd4fbde9d377079951b91944755 test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8deedcff98a55c87b5e473890b2e7a3b16 Add checkBlock to Mining interface (Sjors Provoost)
6077157531c1cec6dea8e6f90b4df8ef7b5cec4e ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed82b1584abb07c0387db0d924c4c0cab validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e57232867d946bc35769632fed49e1bf1464f
TheCharlatan:
ACK a18e57232867d946bc35769632fed49e1bf1464f
ryanofsky:
Code review ACK a18e57232867d946bc35769632fed49e1bf1464f just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7 thread-safety: fix annotations with REVERSE_LOCK (Cory Fields)
aeea5f0ec112f9ec29da37806925e961c4998365 thread-safety: add missing lock annotation (Cory Fields)
832c57a53410870471a52647ce107454de3bc69c thread-safety: modernize thread safety macros (Cory Fields)
Pull request description:
This is one of several PRs to cleanup/modernize our threading primitives.
While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly.
Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference between the old `unlock_function` and new `release_capability`, where our `reverse_lock` only works with the latter. I believe this is an upstream bug. I've [reported and attempted a fix here](https://github.com/llvm/llvm-project/pull/139343), but either way it makes sense to me to modernize.
The second adds a missing annotation pointed out by a fixed `REVERSE_LOCK`. Because clang's thread-safety annotations aren't passed through a reference to `UniqueLock` as one may assume (see [here](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis) for more details), `cs_main` has to be listed explicitly as a requirement.
The last commit actually fixes the `reverse_lock` by making it a `SCOPED_LOCK` and using the pattern [found in a clang test](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3126). Though the docs don't describe how to accomplish it, the functionality was added [in this commit](6a68efc959). Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing `REVERSE_LOCK`s have been updated. To ensure that the mutexes actually match, a runtime assertion is added.
ACKs for top commit:
fjahr:
re-ACK a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7
davidgumberg:
reACK a201a99f8c
theuni:
Ok, done. Those last pushes can be ignored. ACKs on a201a99 are still fresh.
ryanofsky:
Code review ACK a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7. Just dropping 0065b9673db5da2994b0b07c1d50ebfb19af39d0 and fixing incorrect `reverse_lock::lockname` initialization since last review.
TheCharlatan:
Re-ACK a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7
Tree-SHA512: 2755fae0c41021976a1a633014a86d927f104ccbc8014c01c06dae89af363f92e5bc5d4276ad6d759302ac4679fe02a543758124d48318074db1c370989af7a7
Without proper annotations, clang thinks that mutexes are still held for the
duration of a reverse_lock. This could lead to subtle bugs as
EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't.
As mentioned in the docs [0], clang's thread-safety analyzer is unable to deal
with aliases of mutexes, so it is not possible to use the lock's copy of the
mutex for that purpose. Instead, the original mutex needs to be passed back to
the reverse_lock for the sake of thread-safety analysis, but it is not actually
used otherwise.
[0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
Comments are expanded.
Return BlockValidationState instead of passing a reference.
Lock Chainman mutex instead of cs_main.
Remove redundant chainparams and pindexPrev arguments.
Drop defaults for checking proof-of-work and merkle root.
The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.
Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.
Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.
There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().
The final assert is changed into Assume, with a LogError.
Co-authored-by: <Ryan Ofsky <ryan@ofsky.org>
Dropped the default expected_hash parameter from `ReadBlock()`.
In `blockmanager_flush_block_file` tests, we pass {} since the tests would already fail at PoW validation for corrupted blocks.
In `ChainstateManager::LoadExternalBlockFile`, we pass {} when processing child blocks because their hashes aren't known beforehand.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
fab1e02086ceebd7d96417b7a386fe61158bfda9 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4b218fb4853088328b9b488de18dcd2 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdfdf228354b4072b72f4c0ef90fdaade test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
pinheadmz:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
sipa:
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
09ee8b7f278627b917f0784adf23cbc76cae5fa0 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf173210fa1ba7b28fd312b2e62bc9ff45af598 test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)
Pull request description:
Eliminate one block header hash calculation per block-read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
ACKs for top commit:
maflcko:
lgtm ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
achow101:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
jonatack:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
pinheadmz:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4
Eliminate one SHA‑256 double‑hash computation of the header per block read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
ee045b61efc1479c1866b786661ef39a863677d0 rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c3729d4e054ac4260b344a75ad4b7239b3 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06709212934c521c513bb2e1a521a31f psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f80e998f7402433572b695f589f7f42 psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337ad75390a1f8810d6715f3634ed07e98 wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49b2e709a2b00af92ca76e9f30e47aec psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a11825694856a2643e9600fa537182fbb597c107 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4ada5b64c8113450ed736a2581c97518 wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923b10f0690de9dcd34f17fbb8d6de5eb psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd815325713d64b9daa61c2f93061d27bd47d tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d6123e0056656e406cd9f35aac6f71df4b psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
rkrux:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
fjahr:
Code review ACK ee045b61efc1479c1866b786661ef39a863677d0
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too
high value. Since this setting is performance critical, pick an arbitrary value
higher than for -maxmempool but still reasonable.
32-bit architecture is limited to 4GiB, so it doesn't make sense to set a too high value. 500 MB is
chosen as an arbitrary maximum value that seems reasonable.
SignPSBTInput will need to report the specific things that caused an
error to callers, so change it to return a PSBTError. Additionally some
callers will now check the return value and report an error to the user.
Currently, this should not change any behavior as the things that
SignPBSTInput will error on are all first checked by its callers.
62fc42d475df4f23bd93313f95ee7b7eb0d4683f interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7bc9ca155692ac949be2e61c0598a97 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e652885b9e0aec8e62a1bf9590052b320 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a841153510971f0236c527d1a499649 interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776bbe002ee624ec2c09d7c3f981be1b17 interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq)
Pull request description:
#### Motivation
In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests.
However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself.
#### What the PR Does
This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions:
1. Remove rundundant coinbase fee check in `waitNext`
2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing.
3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function.
4. Move `GetTip` implementation to miner.
5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip.
#### Behavior Change
- We now only `Assert` for a valid chainman and notifications pointer once.
ACKs for top commit:
achow101:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
Sjors:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
ryanofsky:
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
- This commit creates a function `WaitTipChanged` that waits for the connected
tip to change until timeout elapsed.
- This function is now used by `waitTipChanged`
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
- Create a new function `AddMerkleRootAndCoinbase` that compute the
block's merkle root, insert the coinbase transaction and the merkle
root into the block.
