Skip to content

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Oct 13, 2024

Additional Information

  • When backporting 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) (the legacy best block is erased before the DIP3 condition is checked, source) but while it completed the transaction (source), critically, it didn't write it to disk (example of writing to disk, here).

    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).

    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, version v0.17 was used as the last version to support legacy txindex as the updated txindex format was introduced in dash#4178 (i.e. after v0.17) and the version selected for having migration code in it (note, migration code was removed in dash#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)
  • 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) from DIP1 onwards (source) 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), 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) 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) 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 (5d10b41) in dash#6152 due to the switch to P2SH_OP_TRUE.

    This has been resolved by reverting this particular change.

    Pre-fix test failure:
    $ ./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 | :wavy_dash: `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 | :wavy_dash: `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
    

Breaking changes

None expected

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation (note: N/A)
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

kwvg added 15 commits October 13, 2024 15:45
This was spotted when working on `feature_txindex_compatibility.py`,
attempting to load old databases would fail because `MigrateDBIfNeeded()`
would delete `DB_OLD_BEST_BLOCK`, write `EVODB_BEST_BLOCK`, commit it
but never flush it.

So when `MigrateDBIfNeeded2()` would read it again, it would note the lack
of `DB_OLD_BEST_BLOCK` and conclude it was a failed run and exit. This
is solved by actually flushing our new best block, which would prevent
`MigrateDBIfNeeded2` from raising an objection.
We need to push DIP-0001's activation height ahead as post-DIP-0001
consensus rejects transactions greater than `MAX_STANDARD_TX_SIZE`
(`bad-txns-oversize`) and this test generates transactions nearly 9.5x
that.

We also need to push v20 activation ahead to avoid different subsidy
calculation logic (`bad-cb-amount`).
Aberrant behaviour was first introduced in 5d10b41 (dash#6152), changes
meant for SegWit made it in, reverting them fixes the problem.
…ex results in recoverable blk file corruption
@kwvg kwvg added this to the 22 milestone Oct 15, 2024
@kwvg kwvg marked this pull request as ready for review October 15, 2024 09:37
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7d9ff96

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7d9ff96

@PastaPastaPasta PastaPastaPasta merged commit 0946eec into dashpay:develop Oct 21, 2024
PastaPastaPasta added a commit that referenced this pull request Jun 20, 2025
, bitcoin#24349, bitcoin#24673, bitcoin#24732, bitcoin#24919, bitcoin#24925, bitcoin#25246, bitcoin#26275, bitcoin#24927, bitcoin#24804, bitcoin#19888, bitcoin#26308, bitcoin#24672, bitcoin#23072, bitcoin#24216 (auxiliary backports: part 24)

caebb0c merge bitcoin#24216: improve connect bench logging (Kittywhiskers Van Gogh)
4e28b54 merge bitcoin#23072: Remove unnecessary timing of Callbacks bench (Kittywhiskers Van Gogh)
aae32c3 merge bitcoin#24672: add missing cs_main lock (Kittywhiskers Van Gogh)
08ab065 merge bitcoin#26308: reduce LOCK(cs_main) scope: ~6 times as many requests per second (Kittywhiskers Van Gogh)
0e507b6 merge bitcoin#19888: Improve getblockstats for unspendables (Kittywhiskers Van Gogh)
51de5bf merge bitcoin#24804: Sanity assert GetAncestor() != nullptr where appropriate (Kittywhiskers Van Gogh)
bf0b580 merge bitcoin#24927: Add test util to populate mempool with random transactions (Kittywhiskers Van Gogh)
0b6839f merge bitcoin#26275: Fix crash on deriveaddresses when index is 2147483647 (2^31-1) (Kittywhiskers Van Gogh)
893bbb4 merge bitcoin#25246: Revert "build: more robustly check for fcf-protection support" (Kittywhiskers Van Gogh)
3c1ca48 merge bitcoin#24925: make GetRand a template, remove GetRandInt (Kittywhiskers Van Gogh)
c85dfae merge bitcoin#24919: Fix a link to `test/lint/lint-python.py` (Kittywhiskers Van Gogh)
e421753 merge bitcoin#24732: Remove buggy and confusing IncrementExtraNonce (Kittywhiskers Van Gogh)
51a68b0 merge bitcoin#24673: followup of remove -deprecatedrpc=addresses flag (Kittywhiskers Van Gogh)
d4fc4dd test: remove vestigial `-deprecatedrpc=addresses` from test, cleanup (Kittywhiskers Van Gogh)
e89f568 merge bitcoin#24349: Split script formatting from script fuzz target (Kittywhiskers Van Gogh)
4630c54 merge bitcoin#24625: Replace struct update_fee_delta with lambda (Kittywhiskers Van Gogh)
1573a82 merge bitcoin#24636: Exclude descriptor when address is excluded (Kittywhiskers Van Gogh)
6393c30 merge bitcoin#23732: Remove gArgs from bdb.h and sqlite.h (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * In [bitcoin#23732](bitcoin#23732), the automatic wallet backup logic ([source](https://github.com/dashpay/dash/blob/6393c306f8e2e7540eac685c96dd0aa92350e03b/src/wallet/wallet.cpp#L3303)) will ignore `-privdb` and use shared memory by default (this is the default behavior for `-privdb`) as `CWallet::AutoBackupWallet()`'s callers do not have access to database options or the context needed to read them ([example](https://github.com/dashpay/dash/blob/6393c306f8e2e7540eac685c96dd0aa92350e03b/src/wallet/wallet.cpp#L377)).

  * When backporting [bitcoin#24636](bitcoin#24636), the changes to the RPC description of `decodepsbt` were not applied as we don't support returning a `witness_utxo` object on account of not supporting SegWit.

  * The `-deprecatedrpc=addresses` argument was removed in [bitcoin#22650](bitcoin#22650) (9faa63f in [dash#6569](#6569)), a leftover in `feature_dip3_deterministicmns.py` was tidied up before backporting [bitcoin#24673](bitcoin#24673).

  * In [bitcoin#24732](bitcoin#24732), the changes meant for `feature_blockfilterindex_prune.py` are applicable for `feature_index_prune.py` due to the OOO backport of [bitcoin#21726](bitcoin#21726) (baf6e26 in [dash#6327](#6327)).
    * `TestChainSetup`'s logic to replace `IncrementExtraNonce()` is meant to replicate the behavior of `IncrementExtraNonce()` to avoid invalidating the hardcoded checkpoints already generated ([source](https://github.com/dashpay/dash/blob/2e144b00fe0d5294182dbca9e46b168be4d57a9b/src/test/util/setup_common.cpp#L392-L405)).

  ## 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:
  PastaPastaPasta:
    utACK caebb0c
  UdjinM6:
    utACK caebb0c

Tree-SHA512: a02dc719e6ddd9ba4649ba7b1a46d6bef0c766c2bd274d3ad683318fba7532147cd70c97d3f560996d63234d7a0906b24dae8d68cff48477f6a98d69d6e25785
PastaPastaPasta added a commit that referenced this pull request Jun 29, 2025
, bitcoin#23075, bitcoin#24817, bitcoin#25087, bitcoin#24839, bitcoin#20456, bitcoin#26640, bitcoin#26892, bitcoin#26280, bitcoin#26657, bitcoin#26923, partial bitcoin#19937 (test backports: part 3)

54740ae merge bitcoin#26923: simplify p2p_{tx_download,eviction}.py by using MiniWallet (Kittywhiskers Van Gogh)
ace58fd merge bitcoin#26657: Run feature_bip68_sequence.py with MiniWallet (Kittywhiskers Van Gogh)
2f7b3b5 merge bitcoin#26280: Return coinbase flag in scantxoutset (Kittywhiskers Van Gogh)
314c28a merge bitcoin#26892: simplify p2p_permissions.py by using MiniWallet (Kittywhiskers Van Gogh)
2c1e298 merge bitcoin#26640: Run mempool_compatibility.py with MiniWallet (Kittywhiskers Van Gogh)
9937fb8 merge bitcoin#20456: Fix intermittent issue in mempool_compatibility (Kittywhiskers Van Gogh)
6add1c4 merge bitcoin#24839: use MiniWallet for mining_prioritisetransaction.py (Kittywhiskers Van Gogh)
69abcec merge bitcoin#25087: use MiniWallet for feature_dbcrash.py (Kittywhiskers Van Gogh)
917f6b6 merge bitcoin#24817: use MiniWallet for feature_fee_estimation.py (Kittywhiskers Van Gogh)
30b1637 merge bitcoin#23075: Fee estimation functional test cleanups (Kittywhiskers Van Gogh)
8d5883c merge bitcoin#24623: Add diamond-shape prioritisetransaction test (Kittywhiskers Van Gogh)
ae7e4cb partial bitcoin#19937: signet mining utility (Kittywhiskers Van Gogh)
1770877 merge bitcoin#24637: use MiniWallet for mempool_package_onemore.py (Kittywhiskers Van Gogh)
4865a3c merge bitcoin#24587: use MiniWallet for rpc_createmultisig.py (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6725

  * In [bitcoin#24587](bitcoin#24587), `checkbalances()` in `rpc_createmultisig.py` had to be reworked as we have to _exclude_ the 9 blocks generated by the test itself (excluding the original mining of 149 blocks), i.e. 2 blocks generated by `do_multisig()` ([source](https://github.com/dashpay/dash/blob/4865a3c6228ccb5d08865ad78f16854c25143f35/test/functional/rpc_createmultisig.py#L171), [source](https://github.com/dashpay/dash/blob/4865a3c6228ccb5d08865ad78f16854c25143f35/test/functional/rpc_createmultisig.py#L194)) (itself called 4 times, [source](https://github.com/dashpay/dash/blob/4865a3c6228ccb5d08865ad78f16854c25143f35/test/functional/rpc_createmultisig.py#L58-L61)) and 1 block generated by `checkbalances()` ([source](https://github.com/dashpay/dash/blob/4865a3c6228ccb5d08865ad78f16854c25143f35/test/functional/rpc_createmultisig.py#L111))

  * [bitcoin#19937](bitcoin#19937) is partially backported to avoid the need for reconnections (or nodes altogether) just to be able to call `getblocktemplate`, as is done in `mining_prioritisetransaction.py`.

  * Portions of [bitcoin#25445](bitcoin#25445) (fa04ff6) have been included when backporting [bitcoin#24817](bitcoin#24817) and [bitcoin#25087](bitcoin#25087) for correctness.

  * Portions of [bitcoin#25435](bitcoin#25435) (fa8421b) and [bitcoin#25356](bitcoin#25356) (fa779de) have been included when backporting [bitcoin#24839](bitcoin#24839) for correctness.

  * In [bitcoin#26923](bitcoin#26923), `rescan_utxos()` has to be manually called (instead of it being called implicitly) as [bitcoin#26886](bitcoin#26886) has not been backported yet and is not included here due to its longer list of dependent PRs.

  * In [bitcoin#20456](bitcoin#20456), v18 has been used to replace v0.15 as `getmempoolinfo["loaded"]` was implemented in [bitcoin#15323](bitcoin#15323) (71e38b9 in [dash#4609](#4609)), which is included in v18.
    * Additionally, [bitcoin#26640](bitcoin#26640) needs a more recent version of Dash Core to be tested against as v0.15 does not recognize `maxfeerate` as an argument ([build](https://github.com/dashpay/dash/actions/runs/15785409773/job/44513474715?pr=6726#step:6:861)), which the `MiniWallet` changes introduced by [bitcoin#26640](bitcoin#26640) rely on.

    * The deletion of the default data directory to avoid problematic migration logic has been documented in the description on [dash#6327](#6327)

  ## Breaking Changes

  None expected. New field added in `scantxoutset` and changes in `getblocktemplate` behavior on test chains are not breaking.

  ## 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:
  PastaPastaPasta:
    utACK 54740ae
  UdjinM6:
    utACK 54740ae

Tree-SHA512: a8c15e1a18f31fc8df7e5445e9f075003e961380114c72226c70dbab0d866d43beb372421ee2e6ce823953af793de3a1112a15d962be27fe7500c5c392171512
PastaPastaPasta added a commit that referenced this pull request Oct 13, 2025
… `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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants