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: block_filter: refresh_snapshot() in build_filter_data may race with ChainService #3759

Merged

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Dec 15, 2022

What problem does this PR solve?

Problem Summary:

block-filter::build_filter_data call refresh_snapshot on every new_block_watcher.changed() here:

self.shared.refresh_snapshot();

and ChainService may update snapshot simultaneously in here:

ckb/chain/src/chain.rs

Lines 480 to 486 in d3fc58f

let new_snapshot =
self.shared
.new_snapshot(tip_header, total_difficulty, epoch, new_proposals);
self.shared.store_snapshot(Arc::clone(&new_snapshot));

For example:

  1. In fn build_filter_data, refresh_snapshot load a Snapshot with tip_header(100)
  2. ChainService received a new_tip_header(101), and store it into snapshot
  3. In fn build_filter_data, refresh_snapshot store the Snapshot with tip_header(100), this cause tip_header goes backwards.

What's Changed:

remove refresh_snapshot() in block-filter::build_filter_data

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Release note

None: Exclude this PR from the release note.

@eval-exec eval-exec marked this pull request as ready for review December 15, 2022 08:50
@eval-exec eval-exec requested a review from a team as a code owner December 15, 2022 08:50
@eval-exec eval-exec requested review from zhangsoledad and removed request for a team December 15, 2022 08:50
@eval-exec eval-exec changed the title Fix: blcok_filter: refresh_snapshot() in build_filter_data may race with ChainService Fix: block_filter: refresh_snapshot() in build_filter_data may race with ChainService Dec 15, 2022
@eval-exec
Copy link
Collaborator Author

eval-exec commented Dec 16, 2022

The CI job was failed after I removed refresh_snapshot() in block-filter::build_filter_data.

When a new tip block comes, ChainService will get a new RocksDB's StoreSnapshot and store it to snapshot after commit the new tip block into RocksDB. Then block-filter get notified and build filter data for the new tip block.

In build_filter_data_for_block, it just only commit the block filter data into rocksdb via StoreTransaction, but the snapshot's inner value(RocksDBSnapshot) doesn't contains the new tip block's block filter data.

let filter_data = filter_writer.into_inner();
let db_transaction = db.begin_transaction();
db_transaction
.insert_block_filter(&header.hash(), &filter_data.pack())
.expect("insert_block_filter should be ok");
db_transaction.commit().expect("commit should be ok");

but in ChainRpc::get_block_filter, it will load a snapshot which doesn't contains the new tip blocks' block filter data, since I removed refresh_snapshot() from block-filter::build_filter_data. So the CI job failed.

ckb/rpc/src/module/chain.rs

Lines 1646 to 1653 in cd1e2f8

fn get_block_filter(&self, block_hash: H256) -> Result<Option<JsonBytes>> {
let snapshot = self.shared.snapshot();
let block_hash = block_hash.pack();
if !snapshot.is_main_chain(&block_hash) {
return Ok(None);
}
Ok(snapshot.get_block_filter(&block_hash).map(Into::into))
}

@eval-exec eval-exec force-pushed the fix/block-filter-snapshot-race branch 4 times, most recently from dda31c7 to 521c80e Compare December 16, 2022 02:21
1. `refresh_snapshot` in block_filter may cause race with `ChainService`
2. get block filter data from ChainDB instead of Snapshot

Signed-off-by: Eval EXEC <execvy@gmail.com>

Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the fix/block-filter-snapshot-race branch from 521c80e to 946b470 Compare December 16, 2022 02:47
@quake
Copy link
Member

quake commented Dec 16, 2022

bors r=quake,driftluo

@bors
Copy link
Contributor

bors bot commented Dec 16, 2022

@bors bors bot merged commit 4bdacfc into nervosnetwork:develop Dec 16, 2022
@doitian doitian mentioned this pull request Dec 16, 2022
@eval-exec eval-exec deleted the fix/block-filter-snapshot-race branch December 21, 2022 08:15
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