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

[Feature] merge state update to single transaction #258

Merged
merged 27 commits into from
Dec 17, 2022

Conversation

0xcb9ff9
Copy link

Description

all trie state update merge in single exclusive transaction

Changes include

  • New feature (non-breaking change that adds functionality)

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

environment:

  • set up 4 validator network
  • use script to send multiple transactions

Test 1:

  • stop a node randomly
  • wait 1 minute
  • restart stop node
  • check whether node is synced

Test 2:

  • kill a node randomly
  • wait 1 minute
  • restart killed node
  • check whether node is synced

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #258 (b65fb27) into dev (a840ee8) will decrease coverage by 2.79%.
The diff coverage is 40.65%.

@@            Coverage Diff             @@
##              dev     #258      +/-   ##
==========================================
- Coverage   51.21%   48.41%   -2.80%     
==========================================
  Files         117      122       +5     
  Lines       17525    18820    +1295     
==========================================
+ Hits         8975     9112     +137     
- Misses       7789     8935    +1146     
- Partials      761      773      +12     
Impacted Files Coverage Δ
blockchain/storage/kvstorage/kvstorage.go 54.16% <ø> (+2.46%) ⬆️
consensus/ibft/hooks.go 55.26% <ø> (ø)
consensus/ibft/pos.go 0.00% <0.00%> (ø)
helper/keccak/keccak.go 0.00% <ø> (ø)
helper/keccak/pool.go 0.00% <ø> (ø)
jsonrpc/debug_endpoint.go 36.84% <0.00%> (ø)
network/bootnodes.go 100.00% <ø> (ø)
network/connections.go 98.07% <ø> (ø)
network/e2e_testing.go 63.76% <0.00%> (-11.77%) ⬇️
protocol/status.go 0.00% <0.00%> (ø)
... and 93 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@DarianShawn DarianShawn added feature New update to Dogechain bug fix Functionality that fixes a bug labels Dec 5, 2022
@DarianShawn
Copy link
Collaborator

@0xcb9ff9
Great work.
The lack of transaction-level trie updates has been a headache for PolygonEdge and Dogechain for a long time.
Your contribution puts an end to the misery.

It's a little flawed whether there are more e2e (end-to-end) test cases on transaction recovery in case of a crash than manual testing.

Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Some confusing issues to solve.

state/immutable-trie/hasher.go Outdated Show resolved Hide resolved
state/immutable-trie/storage.go Outdated Show resolved Hide resolved
state/immutable-trie/storage.go Outdated Show resolved Hide resolved
state/immutable-trie/storage.go Show resolved Hide resolved
state/immutable-trie/storage.go Outdated Show resolved Hide resolved
state/immutable-trie/state.go Outdated Show resolved Hide resolved
state/immutable-trie/state.go Outdated Show resolved Hide resolved
state/immutable-trie/state.go Outdated Show resolved Hide resolved
state/immutable-trie/state.go Outdated Show resolved Hide resolved
state/immutable-trie/trie.go Outdated Show resolved Hide resolved
@0xcb9ff9 0xcb9ff9 force-pushed the state-transaction branch 3 times, most recently from e7a1f3c to 2c9d7fd Compare December 14, 2022 06:47
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Great work.
A little more crafting would make it even better.

state/immutable-trie/statedb.go Outdated Show resolved Hide resolved
state/immutable-trie/metrics.go Show resolved Hide resolved
state/immutable-trie/statedb.go Outdated Show resolved Hide resolved
state/immutable-trie/statedb_transaction.go Show resolved Hide resolved
state/immutable-trie/txn.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Here are some suggestions.

state/immutable-trie/storage.go Show resolved Hide resolved
state/immutable-trie/statedb_transaction.go Show resolved Hide resolved
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Great work. 🙌
Only one last thing: a unit test in protocol failed which shouldn't?

@DarianShawn DarianShawn merged commit 088a2d5 into dogechain-lab:dev Dec 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2022
@0xcb9ff9 0xcb9ff9 deleted the state-transaction branch December 30, 2022 06:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug feature New update to Dogechain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants