-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: merge bitcoin#22772, #22987, #23185, #23157, #23173, #23249, #23211, #22677, #23649, #23683, #23636, #22626 (auxiliary backports: part 17) #6296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
23173: missing changes in src/node/transaction.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment for 22677
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK df448b6
clang-format complains a lot: https://github.com/dashpay/dash/actions/runs/11169142262/job/31049181916?pr=6296
|
This pull request has conflicts, please rebase. |
|
I think we should apply (at least most of) the clang-format recommendations |
|
This pull request has conflicts, please rebase. |
`KeyIDHasher` was never introduced as Dash uses different HD logic. Therefore, there is no `KeyIDHasher` to remove.
…ency on validation
We need to extend `bypass_limits` to `ProcessTransaction()` because Dash allows the `sendrawtransaction` RPC to bypass limits with the optional `bypasslimits` boolean (introduced in dash#2110). The bool arguments are not in alphabetical order to prevent breakage with Bitcoin code that expects `bypass_limits` to always be false.
`ParseHDKeypath` was moved to `bip34.cpp` in bitcoin#14021 (dash#4695), remove lingering definition from `util/strencodings.h`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 8c3ff61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 8c3ff61
, bitcoin#23223, bitcoin#23564, bitcoin#23538, bitcoin#23437, bitcoin#23630, bitcoin#23465, bitcoin#23738, bitcoin#17631, bitcoin#22875 (auxiliary backports: part 18) 5aceee3 merge bitcoin#22875: Fix Racy ParseOpCode function initialization (Kittywhiskers Van Gogh) 427d07f merge bitcoin#17631: Expose block filters over REST (Kittywhiskers Van Gogh) d60f15e merge bitcoin#23738: improve logging of ChainstateManager snapshot persistance (Kittywhiskers Van Gogh) 8725734 merge bitcoin#23465: Remove CTxMemPool params from ATMP (Kittywhiskers Van Gogh) d2cbdc4 merge bitcoin#23630: Remove GetSpendHeight (Kittywhiskers Van Gogh) 8bdab4d merge bitcoin#23437: AcceptToMemoryPool (Kittywhiskers Van Gogh) 1f4e8a0 merge bitcoin#23538: Remove strtol in torcontrol (Kittywhiskers Van Gogh) 2318d9f merge bitcoin#23564: don't use deprecated brew package names (Kittywhiskers Van Gogh) 3b7a739 merge bitcoin#23223: Disable lock contention logging in checkqueue_tests (Kittywhiskers Van Gogh) b383609 merge bitcoin#23227: Avoid treating integer overflow as OP_0 (Kittywhiskers Van Gogh) 0188d32 merge bitcoin#23213: Return error when header count is not integral (Kittywhiskers Van Gogh) eb9e208 merge bitcoin#23156: Remove unused ParsePrechecks and ParseDouble (Kittywhiskers Van Gogh) 18fff7e rpc: switch to taking an integer for `rate` in `quorum dkgsimerror` (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6288 * Dependent on #6296 ## Breaking changes - `quorum dkgsimerror` will no longer accept a decimal value between 0 and 1 for the `rate` argument, it will now expect an integer between 0 to 100. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5aceee3 UdjinM6: utACK 5aceee3 knst: utACK 5aceee3 Tree-SHA512: 8fc34b05a74f2ddbe84b2a7a54772e49941042c89bc74d71d33711e658754a3d086af11fb2437d2bb72ede0c611adc57b82193783e7b6f10fbd4ebab2a7fa7cb
, bitcoin#24626, bitcoin#21726, bitcoin#25123, bitcoin#25074, bitcoin#24832, bitcoin#26215, bitcoin#24858, bitcoin#26417, bitcoin#16981 (index backports) 7d9ff96 merge bitcoin#16981: Improve runtime performance of --reindex (Kittywhiskers Van Gogh) e531dff merge bitcoin#26417: fix intermittent failure in feature_index_prune.py (Kittywhiskers Van Gogh) b04b71a merge bitcoin#24858: incorrect blk file size calculation during reindex results in recoverable blk file corruption (Kittywhiskers Van Gogh) 9e75b99 merge bitcoin#26215: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Kittywhiskers Van Gogh) 3bd584c merge bitcoin#24832: Verify the block filter hash when reading the filter from disk (Kittywhiskers Van Gogh) e507a51 fix: avoid `mandatory-script-verify-flag-failed` crash in bench test (Kittywhiskers Van Gogh) a86109a merge bitcoin#25074: During sync, commit best block after indexing (Kittywhiskers Van Gogh) e6867a3 merge bitcoin#25123: Fix race condition in index prune test (Kittywhiskers Van Gogh) baf6e26 merge bitcoin#21726: Improve Indices on pruned nodes via prune blockers (Kittywhiskers Van Gogh) c65ec19 merge bitcoin#24626: disallow reindex-chainstate when pruning (Kittywhiskers Van Gogh) bcd24a2 fix: push activation height for forks ahead, fix `feature_pruning.py` (Kittywhiskers Van Gogh) 1020356 merge bitcoin#24812: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro (Kittywhiskers Van Gogh) 1caaa85 merge bitcoin#24138: Commit MuHash and best block together for coinstatsindex (Kittywhiskers Van Gogh) b218f12 merge bitcoin#23046: Add txindex migration test (Kittywhiskers Van Gogh) ebae59e fix: make sure we flush our committed best block in no-upgrade cases (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * When backporting [bitcoin#23046](bitcoin#23046), it was discovered that there has been a longstanding bug in `CDeterministicMNManager::MigrateDBIfNeeded`(`2`)`()` that flagged a database taken from an older version for failing its "previous migration attempt", requiring the database to be fully rebuilt through a reindex. This occurred because the older database would be read pre-DIP3 in `MigrateDBIfNeeded()`, which then caused the migration logic to write the new best block ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1236-L1241)) (the legacy best block is erased before the DIP3 condition is checked, [source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1233)) but while it completed the transaction ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1240)), critically, it didn't write it to disk (example of writing to disk, [here](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1288-L1292)). This meant that when it was read again by `MigrateDBIfNeeded2()`, it saw three things a) there is no new best block (because it didn't get written), b) there is no legacy best block (because it gets erased before the new best block is written) and c) that the chain height is greater than 1 (since this isn't a new datadir and the chain has already advanced), it concludes that it was a botched migration attempt and fails ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/evo/deterministicmns.cpp#L1337-L1343)). This bug affects v19 to `develop` (`3f0c2ff3` as of this writing) and prevents `feature_txindex_compatibility.py` from working as expected as it would migrate legacy databases to newer versions to test txindex migration code and get stuck at unhappy EvoDB migration logic, to allow the test to function properly when testing against the latest version of the client, this bug has been fixed as part of this PR. * In [bitcoin#23046](bitcoin#23046), version v0.17 was used as the last version to support legacy txindex as the updated txindex format was introduced in [dash#4178](#4178) (i.e. after v0.17) and the version selected for having migration code in it (note, migration code was removed in [dash#6296](#6296), so far not included as part of any release) was v18.2.2 despite the range being v18.x to v21.x was a) due to the bug mentioned above affecting v19.x onwards and b) v18.2.2 being the latest release in the v18.x lifecycle. * The specific version number used for v0.17 is `170003` as the binaries corresponding to `170000` are not populated in `releases/`, which causes a CI failure ([source](https://gitlab.com/dashpay/dash/-/jobs/8073041955#L380)) * As of `develop` (`3f0c2ff3` as of this writing), `feature_pruning.py` was broken due to changes in Core that were not adjusted for, namely: * The enforcement of `MAX_STANDARD_TX_SIZE` ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/policy/policy.h#L23-L24)) from DIP1 onwards ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L299-L301)) resulting in `bad-txns-oversize` errors in blocks generated for the test as the transactions generated are ~9.5x larger than the now-enforced limit ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L48C51-L48C57)), this is resolved by pushing the DIP1 activation height upwards to `2000` (the same activation height used for DIP3 and DIP8). * Change in subsidy logic in v20 ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/src/validation.cpp#L1073-L1082)) that results in `bad-cb-amount` errors, this is resolved by pushing the v20 activation height upwards. Additionally, an inopportune implicit post-`generate` sync ([source](https://github.com/dashpay/dash/blob/3f0c2ff324fde73bcf3d85c68ce5200d73987eda/test/functional/feature_pruning.py#L215)) also causes the test to fail. Alongside the above, they have been resolved in this PR. * As of `develop` (`3f0c2ff3` as of this writing), `bench_dash` crashes when running the `AssembleBlock` benchmark. The regression was traced back to [bitcoin#21840](bitcoin#21840) (5d10b41) in [dash#6152](#6152) due to the switch to `P2SH_OP_TRUE`. This has been resolved by reverting this particular change. <details> <summary>Pre-fix test failure:</summary> ``` $ ./src/bench/bench_dash Warning, results might be unstable: * CPU governor is '' but should be 'performance' * Turbo is enabled, CPU frequency will fluctuate Recommendations * Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf | ns/op | op/s | err% | ins/op | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:---------- | 17,647,657.00 | 56.66 | 0.1% | 231,718,349.00 | 42,246,265.00 | 0.1% | 0.20 | `AddrManAdd` | 42,201,861.00 | 23.70 | 0.1% | 544,366,811.00 | 102,569,244.00 | 0.0% | 0.46 | `AddrManAddThenGood` | 189,697.83 | 5,271.54 | 0.1% | 1,763,991.40 | 356,189.40 | 0.3% | 0.01 | `AddrManGetAddr` | 454.38 | 2,200,808.04 | 0.6% | 6,229.11 | 1,343.92 | 0.1% | 0.01 | `AddrManSelect` | 1,066,471.00 | 937.67 | 67.6% | 13,350,463.00 | 3,150,465.00 | 0.4% | 0.01 | 〰️ `AddrManSelectByNetwork` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10) | 1,181,774.50 | 846.19 | 49.0% | 18,358,489.50 | 4,224,642.50 | 0.0% | 0.02 | 〰️ `AddrManSelectFromAlmostEmpty` (Unstable with ~1.1 iters. Increase `minEpochIterations` to e.g. 11) bench_dash: bench/block_assemble.cpp:46: void AssembleBlock(benchmark::Bench &): Assertion `res.m_result_type == MempoolAcceptResult::ResultType::VALID' failed. [1] 2343746 IOT instruction (core dumped) ./src/bench/bench_dash ``` </details> ## Breaking changes None expected ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 7d9ff96 PastaPastaPasta: utACK 7d9ff96 Tree-SHA512: e2f1e58abb0a0368c4f1d5e488520957e042e6207b7d2d68a15eb18662405a3cdac91c5ff8e93c8a94c0fdab9b1412bd608d055f196230506c1640439939c25d
…ng sequence between {,non-}unittest codepaths)
1974651 refactor: move remaining `LogPrintf` usage outside (Kittywhiskers Van Gogh)
04dbaa8 style-only: Remove redundant scope in *Chainstate (Kittywhiskers Van Gogh)
872158d Remove all #include // for * comments (Kittywhiskers Van Gogh)
09ab629 test/setup: Use LoadChainstate (Kittywhiskers Van Gogh)
459f339 node/chainstate: extract Dash post-`InitializeChainstate` logic (Kittywhiskers Van Gogh)
c06e074 node/chainstate: Add options for in-memory DBs (Kittywhiskers Van Gogh)
52bb35d node/caches: Remove intermediate variables (Kittywhiskers Van Gogh)
4ab1827 node/caches: Extract cache calculation logic (Kittywhiskers Van Gogh)
d7f1e23 validation: VerifyDB only needs Consensus::Params (Kittywhiskers Van Gogh)
c405492 node/chainstate: Decouple from ShutdownRequested (Kittywhiskers Van Gogh)
fdf803d node/chainstate: Decouple from GetTime (Kittywhiskers Van Gogh)
f7aef8d init: Delay RPC block notif until warmup finished (Kittywhiskers Van Gogh)
94c0ceb Move -checkblocks LogPrintf to AppInitMain (Kittywhiskers Van Gogh)
d3345ee node/chainstate: Reduce coupling of LogPrintf (Kittywhiskers Van Gogh)
a141f5d node/chainstate: Decouple from concept of uiInterface (Kittywhiskers Van Gogh)
913411e Split off VerifyLoadedChainstate (Kittywhiskers Van Gogh)
53231ca node/chainstate: Remove do/while loop (Kittywhiskers Van Gogh)
2ea1bbc Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Kittywhiskers Van Gogh)
29c7362 Move mempool nullptr Assert out of LoadChainstate (Kittywhiskers Van Gogh)
7071282 node/chainstate: Decouple from concept of NodeContext (Kittywhiskers Van Gogh)
ee9d3dd node/chainstate: Decouple from ArgsManager (Kittywhiskers Van Gogh)
d7419e4 node/chainstate: Decouple from stringy errors (Kittywhiskers Van Gogh)
9ab08c4 node/chainstate: Decouple from GetTimeMillis (Kittywhiskers Van Gogh)
2455c06 node: Extract chainstate loading sequence (Kittywhiskers Van Gogh)
620146b chore: sync chainstate loading logic with upstream (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on #6296
* Dependent on #6443
* As one of the backport's intentions were to unify code between `init.cpp` and `setup_common.cpp`, Dash-specific initialization code (to the extent that it can be moved out non-disruptively) has been spun out into `DashChainstateSetup{,Close}()` so it can also be used in `setup_common.cpp` and `validation_chainstatemanager_tests.cpp`.
This is also why `DashTestSetup{,Close}()` (now `DashPostChainstateSetup{,Close}()`) was introduced in [dash#5531](#5531).
* `DashChainstateSetup{,Close}()` (as defined in `node/chainstate.cpp`) cannot take `NodeContext` in because `node/chainstate.cpp` is used in `bitcoin-chainstate`, which doesn't include `NodeContext` ([source](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R795-R798)), this is reflected by neither `LoadChainstate` nor `VerifyLoadedChainstate` taking in `NodeContext`.
* To make it less onerous to use in unit tests, `DashChainstateSetup{,Close}()` has been overloaded with a variant that accepts `NodeContext`.
* To remove `LogPrintf` usage in `node/chainstate.cpp`, index enablement reporting has been pulled out of chainstate loading and BLS scheme reporting has been abstracted out using `notify_bls_state`.
## Breaking Changes
None expected.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 1974651
PastaPastaPasta:
utACK 1974651
Tree-SHA512: 8ebfce48dccc6a867339aff9374a4cb8dc7b02b0c17432db2b97c982523d0d9589a87e6527a103f314bf32be176486311ef6dcde1c4d5cdccc1eeb1f80bbb040
… `MIN_PEER_PROTO_VERSION` to `70221`) 3562045 chore: drop `GOVSCRIPT_PROTO_VERSION` checks (Kittywhiskers Van Gogh) c5bfb57 chore: drop `ISDLOCK_PROTO_VERSION` checks (Kittywhiskers Van Gogh) 4096d77 chore: drop `LLMQ_DATA_MESSAGES_VERSION` checks (Kittywhiskers Van Gogh) 8c141f8 chore: drop `MNAUTH_NODE_VER_VERSION` checks (Kittywhiskers Van Gogh) 5d9435d chore: bump `MIN_PEER_PROTO_VERSION` to `70221` (Kittywhiskers Van Gogh) 1d39829 merge bitcoin#28753: Remove feature_txindex_compatibility.py (Kittywhiskers Van Gogh) 8d13227 partial bitcoin#28195: Drop legacy -txindex check (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6838 * `feature_txindex_compatibility.py` uses Dash Core v0.17.0.3, which has `PROTOCOL_VERSION` `70219` ([source](https://github.com/dashpay/dash/blob/eaca69b22c5a5b1ec0a01e003c2d487542062b44/src/version.h#L14)). Due to the minimum protocol version bump, this test will no longer pass (spotted by UdjinM6!) * Legacy txindex checks were introduced in [bitcoin#22626](bitcoin#22626) (see [dash#6296](#6296), part of Dash Core v22) and the test was introduced in [bitcoin#23046](bitcoin#23046) (see [dash#6327](#6327), part of Dash Core v22). * The migration logic _itself_ was added in [bitcoin#13033](bitcoin#13033) (see [dash#4178](#4178), part of Dash Core v18). This means that it _should_ be safe to drop the failing test altogether as we migrated databases 5 major versions ago, the databases are not funds-sensitive and we dropped support for migration a major version ago. ## Breaking Changes Clients below `70221` (`GOVSCRIPT_PROTO_VERSION`) will not be able to connect to Dash Core ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3562045 PastaPastaPasta: utACK 3562045 Tree-SHA512: e1d3f3cde66e99ed93ad339d4f9aab49fda536bfaefb2102b535a25144fb7d660b77239d6125d95e40350b81fd40a38d99e855066e61280b928baef9ded247f4
Additional Information
When backporting bitcoin#23173,
bypass_limitshad to be extended toChainstateManager::ProcessTransaction()as Dash allows thesendrawtransactionRPC to bypass limits with the optionalbypasslimitsboolean (introduced in dash#2110).The bool arguments are not in alphabetical order to prevent breakage with Bitcoin code that expects
bypass_limitsto always befalse.Breaking Changes
None expected.
Checklist