Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: correct is_snapshot_cs in VerifyDB #6219

Merged
merged 1 commit into from
Aug 18, 2024
Merged

fix: correct is_snapshot_cs in VerifyDB #6219

merged 1 commit into from
Aug 18, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Aug 15, 2024

Issue being fixed or feature implemented

Flag is_snapshot_cs has been inverted in bitcoin#21584

Discovered during investigation of issue:

Verifying last 6 blocks at level 3
2024-08-14T14:51:55Z [0%]...*** Found EvoDB inconsistency, you must reindex to continue

So far as code below does:

        if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
            // If pruning or running under an assumeutxo snapshot, only go
            // back as far as we have data.
            LogPrintf("VerifyDB(): block verification stopping at height %d (pruning, no data)\n", pindex->nHeight);
            break;
        }

In case of missing data in evo db we will get instead of "block verification stopping at height" we may get data inconsistency issue.

What was done?

Inverted condition back (same fix in bitcoin#27596)

How Has This Been Tested?

Unit/functional tests doesn't cover it, but they do no fail after fix.

Breaking Changes

N/A

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
  • I have assigned this pull request to a milestone

@jamesob @knst
fix: correct is_snapshot_cs in VerifyDB
bf377d4 
Similar changes in bitcoin#27596: assumeutxo (2)
@knst knst added this to the 21.2 milestone Aug 15, 2024
UdjinM6
UdjinM6 approved these changes Aug 15, 2024
View reviewed changes
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 bf377d4

PastaPastaPasta
PastaPastaPasta approved these changes Aug 16, 2024
View reviewed changes
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 bf377d4

@PastaPastaPasta PastaPastaPasta merged commit 7d8a19e into dashpay:develop Aug 18, 2024
21 checks passed
@knst knst deleted the fix-verifydb-27596 branch August 27, 2024 15:25
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Oct 22, 2024
@PastaPastaPasta
Merge dashpay#6219: fix: correct is_snapshot_cs in VerifyDB
87bebfc 
bf377d4 fix: correct is_snapshot_cs in VerifyDB (James O'Beirne)

Pull request description:

  ## Issue being fixed or feature implemented
  Flag `is_snapshot_cs` has been inverted in bitcoin#21584

  Discovered during investigation of issue:
  ```
  Verifying last 6 blocks at level 3
  2024-08-14T14:51:55Z [0%]...*** Found EvoDB inconsistency, you must reindex to continue
  ```
  So far as code below does:
  ```
          if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
              // If pruning or running under an assumeutxo snapshot, only go
              // back as far as we have data.
              LogPrintf("VerifyDB(): block verification stopping at height %d (pruning, no data)\n", pindex->nHeight);
              break;
          }
  ```
  In case of missing data in evo db we will get instead of "block verification stopping at height" we may get data inconsistency issue.

  ## What was done?
  Inverted condition back (same fix in bitcoin#27596)

  ## How Has This Been Tested?
  Unit/functional tests doesn't cover it, but they do no fail after fix.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK bf377d4
  PastaPastaPasta:
    utACK bf377d4

Tree-SHA512: ac21e6db6e23c4c7dc150fb16171aef47c9f42c29466b403bca7d56ed6faa2fccc41df92e1fabec4d6e9fd56991e152dea168593a4550fc3583631a63009c27f
@UdjinM6 UdjinM6 modified the milestones: 21.2, 21.1.1 Oct 22, 2024
PastaPastaPasta added a commit that referenced this pull request Oct 22, 2024
@PastaPastaPasta
Merge #6339: chore: release v21.1.1
0fcc156 
d627a6e chore: bump version to 21.1.1 (pasta)
5f9700c docs: release notes for v21.1.1 (pasta)
1c00726 Merge #6277: chore: add builder key for kittywhiskers (pasta)
a2bc0f1 Merge #6290: chore: update pasta gpg key to reflect new subkeys (pasta)
167608c Merge #6338: ci: attest results of guix builds (pasta)
6fb4e49 Merge #6197: ci: always build guix, save artifacts (pasta)
c0ca93c Merge #6340: fix: make 6336 compile in v21.1.x branch, using older CHECK_NONFATAL functionality (pasta)
bb96df4 Merge #6336: fix: rpc getblock and getblockstats for blocks with withdrawal transactions (asset unlock) (pasta)
8e70262 Merge #6131: feat: make a support of Qt app to show Platform transfer Tx (pasta)
80ed279 Merge #6328: backport: bitcoin#30131, bitcoin#23258, bitcoin#30504 - fix bild for Ubuntu 24.10 + clang (pasta)
bd772fb Merge #6229: fix: `creditOutputs` in AssetLock tx json output should be an array of objects, not debug strings (pasta)
9bf39a9 Merge #6222: fix: adjust payee predictions after mn_rr activation, add tests (pasta)
87bebfc Merge #6219: fix: correct is_snapshot_cs in VerifyDB (pasta)
a4e6b8a Merge #6208: fix: persist coinjoin denoms options from gui over restarts (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  See commits, backports, release notes, version bump

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK d627a6e
  kwvg:
    ACK d627a6e
  UdjinM6:
    utACK d627a6e
  ogabrielides:
    utACK d627a6e

Tree-SHA512: cde7e40760e16e9f48da8149c3742d18a34029b057405e4d55b87110da96acbcd19b47280451dd7b5ad1ccfc91fde655452cf5f0f0d1e01a41b4c685337c64b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@knst @UdjinM6 @PastaPastaPasta @jamesob