fix(txvalidator): cache CheckSig within one EvalScript run#41
Merged
Conversation
oskarszoon
changed the title
oskarszoon
force-pushed
the
hotfix/checker-cache-master
branch
from
a910832 to
58145b1
Compare
Master-adapted port of the v1.2.3 hot-fix on the release/1.2.3 branch (see #38 for the release/1.2.3 PR with full background and rationale). In master the verify path was renamed from CScriptEngine in core/scriptengine.cpp to CTxValidator in core/txvalidator.cpp as part of the v1.2.4 reorganisation, so the swap site moves from scriptengine.cpp:473 to txvalidator.cpp:505. The bsv::CachingScriptChecker header (core/checker_cache.hpp) is identical between the two PRs. The cache addresses a multi-hour validator hang observed on testnet block 1,451,505 / tx 7bc9a3408dd0c87b835c887a0bce22c20788fc3c4b953929d4367656d80acab5 whose input spends a 490,001-byte locking script of (OP_2DUP OP_CHECKSIGVERIFY) * 245,000 + OP_CHECKSIG. The script is consensus-valid (post-Genesis removes per-script-op and per-script-size limits) and forces N+1=245,001 identical signature verifications. Without the cache each iteration runs a full ECDSA verify plus a full SignatureHash that SHA256-streams the 490 KB scriptCode buffer; the cache collapses iterations 2..245,001 into per-instance hashmap lookups (O(ns) each), turning hours of work into milliseconds. See #38 for the design discussion, the rejected alternatives (CachingTransactionSignatureChecker from script/sigcache.cpp is wrong cache layer + pulls in gArgs dependencies), and the test plan.
Master-adapted port of the same test on release/1.2.3 (#40). Uses bsv::CTxValidator (renamed from CScriptEngine in v1.2.4) but the test shape is identical: build an extended tx whose input spends a previous output with (OP_2DUP OP_CHECKSIGVERIFY) * 50,000 + OP_CHECKSIG, sign it deterministically, time VerifyScript, assert SCRIPT_ERR_OK and elapsed under 5 s. With the bsv::CachingScriptChecker wired (this PR's core patch) the verify returns in well under 100 ms. Without the cache the call takes seconds-to-minutes depending on the runner.
Master port of the same fix on release/1.2.3 (#40). EvalScript reconstructs scriptCode per CheckSig call so pointer-identity caching misses every iteration; bytewise comparison is the correct test. Without this fix the regression test test_repeated_checksig_cache hits its 5 s budget because the cache never hits. With it, N=50,000 completes in ~0.4 s. See #40 for the full diagnostic of the original pointer-identity miss.
Master port of the same fix on release/1.2.3 (#40). Switches to std::cerr + BOOST_CHECK_MESSAGE for diagnostic on failure (default ctest log level suppresses BOOST_TEST_MESSAGE) and reduces N from 50,000 to 10,000 for headroom on slow runners.
ctnguyen
force-pushed
the
hotfix/checker-cache-master
branch
from
58145b1 to
9f653e0
Compare
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Background
Post-Genesis BSV permits arbitrarily large locking scripts and removes the per-script-op limit. A pathological-but-valid pattern is a long chain of identical
(OP_2DUP OP_CHECKSIGVERIFY)pairs against a single sig+pubkey pushed by thescriptSig. The stack stays at[sig, pubkey]after each pair, so the script performsN+1identical signature verifications.Observed on testnet: block
1,451,505, tx7bc9a3408dd0c87b835c887a0bce22c20788fc3c4b953929d4367656d80acab5whose input spends a 490,001-byte locking script with N=245,000. The plainTransactionSignatureCheckerpath runs 245,001 full ECDSA verifications and 245,001 fullSignatureHashcomputations (each SHA256-streaming 490 KB of scriptCode). Validator nodes peg CPU for hours; presents as a hang in the cgo_Cfunc_ScriptEngine_VerifyScript. Reproduced against gobdk v1.2.2 / v1.2.3 / v1.2.4 and pure-Go gobt — not a library version regression.Strategy
bsv::CachingScriptChecker(per-instance) overridesCheckSig. Cache key is(sig hash, pubkey hash, scriptCode pointer+length). Within oneEvalScriptrun on a script with noOP_CODESEPARATORthe scriptCode reference is stable, so pointer+length equality detects identity without re-hashing the 490 KB buffer. OnOP_CODESEPARATOR(or any pointer change) the cache is cleared. FirstCheckSigperforms one full sighash + one ECDSA verify; remaining iterations hit the map in O(ns).Why not wire
script/sigcache.cpp?Same reasoning as #40:
CachingTransactionSignatureChecker::VerifySignaturecaches the ECDSA result, butTransactionSignatureChecker::CheckSigcomputes the full sighash before consulting the cache, so the 245,001 × 490 KB SHA256 still happens. Net save ~12 s, leaving an 85–170 s grind.sigcache.cppis excluded fromcmake/modules/FindBSVSourceHelper.cmake's_minimal_src_files. Wiring it pulls ingArgsfromutil.cpp(application-only list) and cascades.signatureCacheis file-static insigcache.cpp— cannot be initialised externally without an upstream BSV source patch.The per-instance
CachingScriptCheckerhas none of those costs.Pairs with
release/1.2.3.Test plan
libGoBDK_*.aarchives