Uh oh!
There was an error while loading. Please reload this page.
[pull] master from bitcoin:master - #1828
Merged
Merged
Conversation
…ForAllFlags` This is preparatory for the next commit, which introduces Taproot spends that would fail without this rule being enforced. While touching the function, also remove an outdated comment that refers to a parameter that doesn't exist anymore (`upgraded_nop` was removed in commit 01013f5, PR #10699).
The node currently continues low-work headers presync and requests more headers when its clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP. Record this behavior before the follow-up rejects the invalid elapsed-time calculation. The unit test covers HeadersSyncState() behavior while the functional test covers net_processing.cpp behavior. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
We should not proceed syncing headers from peers when the local system clock is incorrectly set. A node with a system clock set too far back will typically fail early during startup when the chainstate detects the tip to be too far in the future. This means that in practice we don't expect the failure to ever happen in net_processing.cpp. An exception is thrown from HeadersSyncState() in order to only compute the error condition once. An alternative would be to compute it a second time in TryLowWorkHeadersSync() to guard against calling HeadersSyncState(), and have an assert inside HeadersSyncState(). We shut down the process so possible resource leaks due to the exception should not be an issue, although none have been spotted. Throwing an exception also keeps the unit test straightforward. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Avoids frequent double negation. See also fa7078d when it was renamed from the previous name, "ValidAsCString()".
…ache_tests.cpp 3ba1bbf test: exercise Schnorr signature cache in txvalidationcache_tests.cpp (Sebastian Falbesoner) 198b36b test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags` (Sebastian Falbesoner) e78a2a0 test: refactor: simplify tx vin/vout creation in txvalidationcache_tests.cpp (Sebastian Falbesoner) Pull request description: The Schnorr verification path of the signature cache is currently never hit in the unit tests, i.e. with the following patch they still pass: ```diff diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp index c6fcc8f..87688c1049 100644 --- a/src/script/sigcache.cpp +++ b/src/script/sigcache.cpp @@ -44,6 +44,7 @@ void SignatureCache::ComputeEntryECDSA(uint256& entry, const uint256& hash, cons void SignatureCache::ComputeEntrySchnorr(uint256& entry, const uint256& hash, std::span<const unsigned char> sig, const XOnlyPubKey& pubkey) const { + assert(false); CSHA256 hasher = m_salted_hasher_schnorr; hasher.Write(hash.begin(), 32).Write(pubkey.data(), pubkey.size()).Write(sig.data(), sig.size()).Finalize(entry.begin()); } ``` This PR adds missing coverage for that by adding a Taproot key-path spend to `checkinputs_test` in `txvalidationcache_tests.cpp`. Same as for the already-existing ECDSA spends, the caching is tested across a large number of flag combinations (using `ValidateCheckInputsForAllFlags`), both with an invalid Schnorr signature (-> should only fail if `SCRIPT_VERIFY_TAPROOT` is set) and a valid one (-> should pass for all flag combinations). ACKs for top commit: Bortlesboat: tACK 3ba1bbf sedited: ACK 3ba1bbf instagibbs: ACK 3ba1bbf Tree-SHA512: e43f7077d9e9ab6f8b5e9e70f0187767d65f686ce24350ce5d61cc4cdf07d5eebdf5e4327ce665c212b7cc02be1e8632a6a9fbcf6be2916f8e058993e5fb2650
15630c7 validation: remove unused m_chainparams from ATMPArgs (fanquake) 84c5290 validation: remove unused args from PolicyScriptChecks (fanquake) a9d5cf7 validation: remove unused args from ConsensusScriptChecks (fanquake) d26dc09 validation: remove unused total_vsize arg from PackageRBFChecks (fanquake) 2cb6c15 validation: remove unused PackageMempoolAcceptResult constructor (fanquake) Pull request description: Remove some unused code from validation. ACKs for top commit: thomasbuilds: ACK 15630c7 sedited: ACK 15630c7 yuvicc: ACK 15630c7 hebasto: ACK 15630c7, completeness of removing unused parameters in the `validation` module verified by overriding the `-Wunused-parameter` compiler flag for `src/validation.cpp`. jeanpablojp: tACK 15630c7 Tree-SHA512: a01ff6ea758132d6ad4c163d51c36d9e2cfaf91e90ca6451323591341fefec23c875af26e0b66e6cdba87ae6cab1418048c8788361d9c62fb8e0400d4dcaeac7
…lock ff3e2e4 net: Trigger process abort when behind start block MTP (Hodlinator) 1883cec test: Characterize lagging-clock headers presync (Hodlinator) Pull request description: ### Problem Headers presync computes `m_max_commitments` from the elapsed time since the chain-start MTP plus `MAX_FUTURE_BLOCK_TIME`. When the local system clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP, that elapsed value is negative, but it is used in arithmetic assigned to the unsigned commitment cap. This can turn the intended zero bound into a large cap, letting low-work headers presync continue instead of aborting when a reasonable commitment cap would have been exceeded. ### Fix Instead of allowing an invalid `HeadersSyncState` object to be created, emit an error and **abort the node process**. Typically, the node will detect that the system clock is set too far in the past when comparing it to the chain tip during chain state loading and shut down before we start syncing headers. So in practice this is very unlikely to make a difference (might be possible if the system clock jumps backwards after we loaded the chain state). #### Commits * Add functional and unit characterization tests [pinning the current behavior](#35260). * The fix, along with corresponding test changes. --- Replaces #35208 which was clamping `m_max_commitments` to zero and then letting the `HeadersSyncState` consume headers until the block height either reached the the next `commitment_period` point and aborted, or reached the minimum work threshold and succeeded (possible when having been offline for >144 blocks). ACKs for top commit: l0rinc: diff and code review ACK ff3e2e4 sedited: ACK ff3e2e4 mzumsande: Code Review ACK [ff3e2e4](ff3e2e4) Tree-SHA512: bdd82fd0609309aa4bea026db1b607ae856c53403ec01b2511fa2ccae9db4ff1bb9e39523b446583c09ae53823275b8a603050d9090b61fabb84fab35e458f28
8d93098 refactor: Replace !ContainsNoNUL() with ContainsNUL() (Hodlinator) Pull request description: Avoids frequent double negation. See also fa7078d when it was renamed from the previous name, "ValidAsCString()". Found while reviewing #35041. ACKs for top commit: maflcko: lgtm ACK 8d93098 l0rinc: code review ACK 8d93098 sedited: ACK 8d93098 janb84: ACK 8d93098 Tree-SHA512: 3ed1d264953f08272c115d760e8149c5985d63331404ac3b1017a277c4b3a61862915851fe45746e26ed46fe7478f851680d205e5ef83e727d061f2867fed99c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )