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: don't use cache if tx-pool does re-org process during hardfork #3079

Merged
merged 1 commit into from
Oct 10, 2021
Merged

fix: don't use cache if tx-pool does re-org process during hardfork #3079

merged 1 commit into from
Oct 10, 2021

Conversation

yangby-cryptape
Copy link
Collaborator

What problem does this PR solve?

  • If the re-org process is running during hardfork, the whole caches for transactions will be cleared.

    ckb/tx-pool/src/process.rs

    Lines 1027 to 1031 in 033c6f9

    let txs_opt = if hardfork_during_detach || hardfork_during_attach {
    // The tx_pool is locked, remove all caches if has any hardfork.
    {
    self.txs_verify_cache.write().await.clear();
    }

  • But the cache for re-add detached transactions is fetched before doing re-org process.

    ckb/tx-pool/src/process.rs

    Lines 1018 to 1020 in 033c6f9

    let retain: Vec<TransactionView> = detached.difference(&attached).cloned().collect();
    let fetched_cache = self.fetch_txs_verify_cache(retain.iter()).await;

  • So the cache for re-add detached transactions will be obsolete.

    ckb/tx-pool/src/process.rs

    Line 1048 in 033c6f9

    self.readd_dettached_tx(&mut tx_pool, retain, fetched_cache);

What is changed and how it works?

If tx-pool is doing re-org process during hardfork, returns an empty HashMap as the cache.

Check List

Tests

  • This feature doesn't have its own tests, but it has already be tested in other integration tests.

Release note

Title Only: Include only the PR title in the release note.

@yangby-cryptape yangby-cryptape requested a review from a team as a code owner October 9, 2021 14:41
@yangby-cryptape
Copy link
Collaborator Author

bors merge=zhangsoledad,driftluo

@quake
Copy link
Member

quake commented Oct 10, 2021

bors r=quake,zhangsoledad,driftluo

@bors
Copy link
Contributor

bors bot commented Oct 10, 2021

Already running a review

@bors
Copy link
Contributor

bors bot commented Oct 10, 2021

Build succeeded:

@bors bors bot merged commit 35e0de6 into nervosnetwork:develop Oct 10, 2021
@yangby-cryptape yangby-cryptape deleted the pr/fix-cache-during-hardfork branch October 10, 2021 03:42
@yangby-cryptape yangby-cryptape mentioned this pull request Oct 13, 2021
Merged
bors bot added a commit that referenced this pull request Oct 18, 2021
@bors @yangby-cryptape
Merge #3090
fee468d 
3090: fix: tx-pool refreshes caches with the incorrect VM version after hardfork r=quake,driftluo a=yangby-cryptape

### What problem does this PR solve?

Tx-pool refreshes caches with the incorrect VM version after hardfork if current tip block is the last block before hardfork.

#### Problem Summary

After #3058, all scripts will be verified under the environment of current tip block.

So when tip block is the last block before hardfork, the hardfork features are not enabled, that means all scripts will be verified with old features.

But in next block, all scripts should be verified with new hardfork features.

#### PR Summary

- test(hardfork): add a script and a unit test to test the change of the script's cycles after upgrade vm0 to vm1

  For test propose, I need a **fixed-cycles** lock script to `assert` and it should has different cycles in vm0 and vm1.
  The cycles of secp256k1 lock is not stable.

  This unit test also ensures the VM version is correct:
  https://github.com/nervosnetwork/ckb/blob/e8f88fd73451c526f37e0879a5ed41272dbd3020/script/src/types.rs#L57-L58

- fix(test): fix `MockChain` that it always panics in `get_block_epoch(..)` when epoch changed

- fix(hardfork): fix tx-pool that it refreshes caches with the incorrect VM version if it re-org to the last block before hardfork

  When the tip is the last block before hardfork, the transactions in block template should be verified with new hardfork features.

  So, we should always verify the transactions which are not committed, with the features for next block.

- test(hardfork): add a unit test for the tx-pool caches (for #3079)

- fix(hardfork): network should enable ckb2021 features when tip is the last block of ckb2019

### Check List

Tests

- Unit test

### Release note

```release-note
Title Only: Include only the PR title in the release note.
```



Co-authored-by: Boyu Yang <yangby@cryptape.com>
bors bot added a commit that referenced this pull request Oct 18, 2021
@bors @yangby-cryptape
Merge #3090
cab368f 
3090: fix: tx-pool refreshes caches with the incorrect VM version after hardfork r=quake,driftluo a=yangby-cryptape

### What problem does this PR solve?

Tx-pool refreshes caches with the incorrect VM version after hardfork if current tip block is the last block before hardfork.

#### Problem Summary

After #3058, all scripts will be verified under the environment of current tip block.

So when tip block is the last block before hardfork, the hardfork features are not enabled, that means all scripts will be verified with old features.

But in next block, all scripts should be verified with new hardfork features.

#### PR Summary

- test(hardfork): add a script and a unit test to test the change of the script's cycles after upgrade vm0 to vm1

  For test propose, I need a **fixed-cycles** lock script to `assert` and it should has different cycles in vm0 and vm1.
  The cycles of secp256k1 lock is not stable.

  This unit test also ensures the VM version is correct:
  https://github.com/nervosnetwork/ckb/blob/e8f88fd73451c526f37e0879a5ed41272dbd3020/script/src/types.rs#L57-L58

- fix(test): fix `MockChain` that it always panics in `get_block_epoch(..)` when epoch changed

- fix(hardfork): fix tx-pool that it refreshes caches with the incorrect VM version if it re-org to the last block before hardfork

  When the tip is the last block before hardfork, the transactions in block template should be verified with new hardfork features.

  So, we should always verify the transactions which are not committed, with the features for next block.

- test(hardfork): add a unit test for the tx-pool caches (for #3079)

- fix(hardfork): network should enable ckb2021 features when tip is the last block of ckb2019

### Check List

Tests

- Unit test

### Release note

```release-note
Title Only: Include only the PR title in the release note.
```



Co-authored-by: Boyu Yang <yangby@cryptape.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quake quake quake approved these changes

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@driftluo driftluo driftluo approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@yangby-cryptape @quake @zhangsoledad @driftluo