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

feat(sync): sender recovery stage #181

Merged
merged 13 commits into from
Nov 23, 2022
Merged

Conversation

rkrasiuk
Copy link
Member

@rkrasiuk rkrasiuk commented Nov 9, 2022

Implement sender recovery stage.

closes #180

blocked by #190 and #204

@rkrasiuk rkrasiuk added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels Nov 9, 2022
@rkrasiuk rkrasiuk marked this pull request as draft November 9, 2022 12:52
@gakonst
Copy link
Member

gakonst commented Nov 9, 2022

@rkrasiuk is this superseded by @rakita PR?

@rkrasiuk
Copy link
Member Author

rkrasiuk commented Nov 9, 2022

@gakonst nope, this is stage implementation, but it's on top of #179

crates/interfaces/src/test_utils.rs Outdated Show resolved Hide resolved
crates/stages/src/stages/senders.rs Show resolved Hide resolved
crates/stages/src/stages/senders.rs Outdated Show resolved Hide resolved
let max_block = input.previous_stage.map(|(_, num)| num).unwrap_or_default();
let mut body_cursor = tx.cursor_mut::<tables::BlockBodies>()?;
let mut body_walker =
body_cursor.walk((start_block, start_hash).into())?.take_while(|res| {
Copy link
Member

Choose a reason for hiding this comment

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

We shoild add a helper method for walkers over range of blocks, maybe even just passing it StageInput

@gakonst
Copy link
Member

gakonst commented Nov 15, 2022

Blocked on @onbjerg @rkrasiuk aligning on whether we should use CumTxCount or Bodies table to get transactions.

@gakonst
Copy link
Member

gakonst commented Nov 17, 2022

@rkrasiuk @onbjerg did u guys sync / agree on next steps here? pls try to unblock this before eod

@onbjerg
Copy link
Member

onbjerg commented Nov 18, 2022

@gakonst Yes, either way is fine for MVP. We can start profiling once we're over that stage as the difference in implementation probably won't differ much, so switching to either one afterwards should be fine.

@gakonst
Copy link
Member

gakonst commented Nov 19, 2022

OK sounds good - thank you! Let's unblock this and move forward with the current approach then @rkrasiuk

@rkrasiuk
Copy link
Member Author

I plan to add some stage db utils in a follow up PR

@rkrasiuk rkrasiuk marked this pull request as ready for review November 21, 2022 17:12
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #181 (b76fad2) into main (027fc2b) will increase coverage by 0.34%.
The diff coverage is 95.67%.

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   73.20%   73.54%   +0.34%     
==========================================
  Files         224      225       +1     
  Lines       20144    20347     +203     
==========================================
+ Hits        14746    14964     +218     
+ Misses       5398     5383      -15     
Impacted Files Coverage Δ
crates/stages/src/test_utils/macros.rs 100.00% <ø> (ø)
crates/stages/src/stages/senders.rs 93.78% <93.78%> (ø)
crates/interfaces/src/test_utils/generators.rs 100.00% <100.00%> (ø)
crates/stages/src/stage.rs 100.00% <100.00%> (ø)
crates/stages/src/stages/bodies.rs 96.03% <100.00%> (+0.21%) ⬆️
crates/stages/src/stages/tx_index.rs 96.26% <100.00%> (-0.07%) ⬇️
crates/stages/src/test_utils/stage_db.rs 100.00% <100.00%> (ø)
crates/stages/src/util.rs 100.00% <100.00%> (ø)
crates/net/discv4/src/lib.rs 68.29% <0.00%> (+0.29%) ⬆️
... and 2 more

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

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge and add the helpers requested in a follow up PR.


let start_hash = tx.get::<tables::CanonicalHeaders>(start_block)?.unwrap();
let mut body_cursor = tx.cursor::<tables::BlockBodies>()?;
let mut body_walker = body_cursor.walk((start_block, start_hash).into())?;
Copy link
Member

Choose a reason for hiding this comment

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

Let's add get body walker by 1. block hash and 2. Block number helpers

let (_, body) = entry?;
for tx_id in body.base_tx_id..body.base_tx_id + body.tx_amount {
let transaction = tx
.get::<tables::Transactions>(tx_id)?
Copy link
Member

Choose a reason for hiding this comment

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

This is a hash right? Normally at least. Are txs are written by number in the test dbs?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's not a hash, that's encoded tx data TransactionSigned

let tx = db.get_mut();

// Look up the hash of the unwind block
if let Some(unwind_hash) = tx.get::<tables::CanonicalHeaders>(input.unwind_to)? {
Copy link
Member

Choose a reason for hiding this comment

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

Should we log if it's none?

hash: unwind_hash,
})?;

unwind_table_by_num::<DB, tables::TxSenders>(tx, latest_tx - 1)?;
Copy link
Member

Choose a reason for hiding this comment

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

We sure this isn't off by one?

Copy link
Member Author

Choose a reason for hiding this comment

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

consider block 1 with 2 transactions and block 2 with 3 transactions. the transactions would be enumerated 0..4 (inclusive). if we need to unwind to block 1, we'd just need to look up the tx count at unwind block (2 in this case) and turn it into the tx_id of the last transaction

crates/stages/src/stages/senders.rs Show resolved Hide resolved
let mut tx_cursor = tx.cursor::<tables::Transactions>()?;
// Walk the transactions from start to end index (exclusive)
let entries = tx_cursor
.walk(start_tx_index)?
Copy link
Member

Choose a reason for hiding this comment

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

We should have helpers for these "range walkers" cursor.walk_range(range: Range)

crates/stages/src/stages/senders.rs Show resolved Hide resolved
crates/stages/src/stages/senders.rs Show resolved Hide resolved
@rkrasiuk
Copy link
Member Author

linter error unrelated

@rkrasiuk rkrasiuk merged commit 82b37b9 into main Nov 23, 2022
@rkrasiuk rkrasiuk deleted the rkrasiuk/stage-sender-recovery branch November 23, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staged-sync Related to staged sync (pipelines and stages) C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sender Recovery Stage
5 participants