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.

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

fix: ensure that accumulated orphan chain data is committed before header validation #3462

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Oct 16, 2021

Description

  • commit orphan chain accumulated data before orphan header validation so that
    accumulated target difficulty is available for chain comparison
  • use hash for reorg chain rewind because the height value of the candidate block is not yet
    checked against the fork height.
  • improve test infra to allow blockchain tests to be easily be constructed
  • add a failing test for the fix in the PR

Motivation and Context

Observed errors:

2021-10-10 09:17:19.987800805 [c::cs::database] WARN  Discarding orphan 0291ba64d777e46016ca7b055bdf6979c1fe11bf31b78a7c20d507cb69c3f293 because it has an invalid header: FatalStorageError("The requested chain_header_in_all_chains was not found via hash:9c2c5734a783d891b617905e27e861ac1595760d5c22335c8d31feb7dc38a2a5 in the database")

The above error occurs because of orphan accumulated data was set in the DbTransaction but is not
actually committed to lmdb. This manifests as a validation error and was not picked up by other tests because
the validator was mocked out.

The solution in this PR is to write the transactions as needed. This means that in a rollback situation, the
accumulated orphan chain data will remain. That should be ok since that data can be overwritten/replaced
on the next reorg evaluation if needed.

This demonstrates the shortcomings of the current approach and the need for the calling code in
BlockchainDatabase<B> to have access to ACID DB transactions.
I have played with the idea of abstracting and generifying atomic transactions but found the need for GAT to get it to work. We no longer
have or use any other BlockchainBackend impl other than LMDB so leaking LMDB Read,WriteTransaction
may be acceptable. Passing the transaction in to every function in BlockchainBackend may be a deal breaker.

How Has This Been Tested?

  • Issue reproduced in handle_possible_reorg::it_links_many_orphan_branches_to_main_chain
  • Manually (not necessarily tested, the base node runs for a day and is still at the tip)

Consolidated some of the existing "blockchain builder" test infra that was duplicated and added
a declarative macro for building chains let specs = block_spec!(["A->GB"], ["B->A", difficulty: 100]);

…ader validation

- commit orphan chain accumulated data before orphan header validation so that
  accumulated target difficulty is available for chain comparison
- use hash for reorg chain rewind because height may potentially be
  manipulated to cause a large rewind
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Nice. It's a little more complicated with all the new individual transactions, but as you said, this should eventually be changed to allow a better transaction model.

@aviator-app aviator-app bot merged commit 80f7c78 into tari-project:development Oct 18, 2021
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 18, 2021
* development:
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 18, 2021
* development:
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
@sdbondi sdbondi deleted the core-fix-next-tip-header branch October 19, 2021 07:17
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 25, 2021
* development: (31 commits)
  feat!: revalidate all outputs (tari-project#3471)
  fix: check SAF message inflight and check stored_at is in past (tari-project#3444)
  feat!: apps should not depend on other app configs (tari-project#3469)
  fix: fix recovery test reporting message (tari-project#3479)
  chore: improve cucumber tests to wait for broadcast (tari-project#3461)
  test: use TCP node for daily sync test (tari-project#3464)
  fix: remove unbounded vec allocations from base node grpc/p2p messaging (tari-project#3467)
  fix: upgrade rustyline dependencies (tari-project#3476)
  fix(dht): discard encrypted message with no destination (tari-project#3472)
  fix: remove consensus breaking change in transaction input (tari-project#3474)
  feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411)
  fix: validate dht header before dedup cache (tari-project#3468)
  fix: sha256sum isn't available on all *nix platforms (tari-project#3466)
  fix: typo in console wallet (tari-project#3465)
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
  fix: remove unnecessary wallet dependency (tari-project#3438)
  test: simplify cucumber tests (tari-project#3457)
  ci: create script to update DNS records from hashes.txt (tari-project#3458)
  ...
@sdbondi sdbondi restored the core-fix-next-tip-header branch February 3, 2022 05:29
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.

2 participants