Refactor: merge block assembly paths for follower and miner #2946

pavitthrap · 2021-11-22T13:00:07Z

Added setup_block and finish_block to consolidate some logic in block assembly.
Refactored following functions:

epoch_begin in miner.rs
append_block in chainstate/stacks/db/blocks.rs

Reviewer notes:

The one change I'm aware of between the existing implementation and my refactor is that the follower used to panic if the call to get_stacks_block_anchored_cost fails (when getting the parent execution cost). The code now returns an error. I can make this panic if need be.

To review this, I went through the code paths of both the follower and miner and ensured all the logic was being executed.

follower: checked that all the lines in append_block are still being called (and in a order that still makes sense).
miner: checked that the logic in epoch_begin and mine_anchored_block is preserved.

Tests:
Since this was a refactor, no new tests were added.

WIP - need to remove double mutable borrow of chainstate Factored out setup_block Renamed block finishing function and added function comment

codecov · 2021-11-22T13:24:56Z

Codecov Report

Merging #2946 (e17fcc1) into develop (591cbfe) will increase coverage by 0.76%.
The diff coverage is 85.38%.

@@             Coverage Diff             @@
##           develop    #2946      +/-   ##
===========================================
+ Coverage    82.15%   82.91%   +0.76%     
===========================================
  Files          235      235              
  Lines       189697   189778      +81     
===========================================
+ Hits        155837   157351    +1514     
+ Misses       33860    32427    -1433

Impacted Files	Coverage Δ
src/blockstack_cli.rs	`83.09% <ø> (ø)`
src/chainstate/stacks/mod.rs	`75.13% <ø> (ø)`
src/net/download.rs	`41.20% <0.00%> (+9.02%)`	⬆️
src/net/p2p.rs	`65.14% <58.33%> (+2.59%)`	⬆️
src/chainstate/stacks/db/blocks.rs	`88.25% <84.23%> (+0.07%)`	⬆️
src/chainstate/stacks/miner.rs	`94.30% <97.08%> (+0.13%)`	⬆️
src/chainstate/coordinator/tests.rs	`95.56% <100.00%> (+0.01%)`	⬆️
src/net/mod.rs	`67.74% <100.00%> (+0.94%)`	⬆️
src/deps/bitcoin/network/serialize.rs	`43.00% <0.00%> (-5.00%)`	⬇️
src/burnchains/bitcoin/network.rs	`77.77% <0.00%> (-3.25%)`	⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b99902...e17fcc1. Read the comment docs.

kantai · 2021-11-23T16:21:36Z

Thanks @pavitthrap ! I'll try to review this PR this week, but I think, given the timing of the 2.05.0.0.0 rc and release, we shouldn't merge this into that release, and wait to merge until the subsequent point release.

src/chainstate/stacks/db/blocks.rs

src/chainstate/stacks/miner.rs

jcnelson · 2021-11-29T15:18:52Z

src/chainstate/stacks/miner.rs

+    /// of the burn tip, burn tip height + 1, the parent microblock stream,
+    /// the parent consensus hash, the parent header hash, and a bool
+    /// representing whether the network is mainnet or not.
+    pub fn pre_epoch_begin<'a>(


I'm having a hard time understanding why the pre_epoch_begin and epoch_begin split needs to exist. It seems like the bulk of the refactoring work here is in creating the StacksChainState::setup_block() method. Is there a compelling reason why the miner can't keep a single epoch_begin() method?

I was also wondering this.

I am pretty sure that this is necessary for the borrow-checker/ownership checks: something higher than the scope of epoch_begin needs to own the returned ChainstateTx and ClarityInstance (returned by chainstate.chainstate_tx_begin()?;), so this code cannot all live in epoch_begin. And that line cannot be called by itself, because a lot of the setup work in pre_epoch_begin needs reference the StacksChainState, which cannot be used while the ChainstateTx and ClarityInstance are alive (because their lifetimes are bound to a mutable reference to the StacksChainState.

It seems like it should be possible to expand the definition of epoch_begin to allow all of this.

But, I also don't feel that strongly, so I am OK with it.

I am pretty sure that this is necessary for the borrow-checker/ownership checks:

Yeah I see that now.

@pavitthrap Can we call this something more informative, in order to communicate this? Perhaps epoch_setup()?

src/chainstate/stacks/miner.rs

gregorycoppola

Thanks! Overall looks like it helps remove code duplication.

I had a few questions.

src/chainstate/stacks/db/blocks.rs

gregorycoppola · 2021-11-30T19:19:43Z

src/chainstate/stacks/db/blocks.rs

+                        "Invalid Stacks microblocks {},{} (offender {}): {:?}",
+                        parent_consensus_hash, parent_header_hash, mblock_header_hash, &e
+                    );
+                    warn!("{}", &msg);


I think ti's easier to read if you put the format in thw warn.

That would require a needless clone() -- you'd have to build the string once for the warn!, and once for the InvalidStacksMicroblock error constructor. The code pattern here is used elsewhere, and is fine by me.

src/chainstate/stacks/db/blocks.rs

gregorycoppola · 2021-11-30T19:52:06Z

src/chainstate/stacks/miner.rs

+    /// of the burn tip, burn tip height + 1, the parent microblock stream,
+    /// the parent consensus hash, the parent header hash, and a bool
+    /// representing whether the network is mainnet or not.
+    pub fn pre_epoch_begin<'a>(


I was also wondering this.

src/chainstate/stacks/miner.rs

gregorycoppola · 2021-12-01T14:40:44Z

I think this change looks good.. the only thing is I think there is some debate about whether pre_epoch_begin is necessary as its own function, especially if the two functions are always called together.

gregorycoppola · 2021-12-01T15:27:23Z

@kantai @jcnelson is it necessary for Rust-based reasons that epoch_begin and pre_epoch_begin must be separate functions?

If they are always called together it would be nice to merge them, to save on people having to know about the correspondence.

kantai · 2021-12-01T15:36:00Z

@kantai @jcnelson is it necessary for Rust-based reasons that epoch_begin and pre_epoch_begin must be separate functions?

Yes, I believe so -- I followed up in the code review thread.

kantai

LGTM! Thanks @pavitthrap

gregorycoppola

Thanks!

blockstack-devops · 2024-11-19T00:22:44Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

factored out logic in from miner.rs - moved it to StacksChainState

b4c26c7

WIP - need to remove double mutable borrow of chainstate Factored out setup_block Renamed block finishing function and added function comment

pavitthrap changed the title ~~factored out logic in from miner.rs - moved it to StacksChainState~~ Refactor: merge block assembly paths for follower and miner Nov 23, 2021

pavitthrap added 2 commits November 23, 2021 12:26

Cleaned up code, added comments

c37d6a1

Minor tweaks, doc change

6fa4e04

pavitthrap marked this pull request as ready for review November 23, 2021 07:17

pavitthrap requested review from kantai, jcnelson and gregorycoppola November 23, 2021 07:17

kantai linked an issue Nov 23, 2021 that may be closed by this pull request

Consolidate block assembly paths for miner and follower #2914

Closed

jcnelson reviewed Nov 23, 2021

View reviewed changes

src/chainstate/stacks/db/blocks.rs Outdated Show resolved Hide resolved

jcnelson reviewed Nov 24, 2021

View reviewed changes

src/chainstate/stacks/miner.rs Outdated Show resolved Hide resolved

jcnelson reviewed Nov 29, 2021

View reviewed changes

src/chainstate/stacks/miner.rs Outdated Show resolved Hide resolved

jcnelson reviewed Nov 29, 2021

View reviewed changes

src/chainstate/stacks/miner.rs Outdated Show resolved Hide resolved

kantai changed the base branch from next-costs to develop November 29, 2021 18:41

gregorycoppola reviewed Nov 30, 2021

View reviewed changes

Addressed comments: added structs for legibility

6962879

pavitthrap requested review from jcnelson and gregorycoppola December 2, 2021 20:57

kantai approved these changes Dec 7, 2021

View reviewed changes

gregorycoppola approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'develop' into refactor/merge-block-processing

e17fcc1

pavitthrap merged commit 194a5b8 into develop Dec 13, 2021

jcnelson mentioned this pull request Feb 7, 2022

Consolidate block assembly paths for miner and follower #2914

Closed

blockstack-devops added the locked label Nov 19, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: merge block assembly paths for follower and miner #2946

Refactor: merge block assembly paths for follower and miner #2946

pavitthrap commented Nov 22, 2021 •

edited

Loading

codecov bot commented Nov 22, 2021 •

edited

Loading

kantai commented Nov 23, 2021 •

edited

Loading

jcnelson Nov 29, 2021

gregorycoppola Nov 30, 2021

kantai Dec 1, 2021 •

edited

Loading

gregorycoppola Dec 1, 2021

jcnelson Dec 7, 2021

gregorycoppola left a comment

gregorycoppola Nov 30, 2021

jcnelson Dec 7, 2021

gregorycoppola Nov 30, 2021

gregorycoppola commented Dec 1, 2021

gregorycoppola commented Dec 1, 2021

kantai commented Dec 1, 2021

kantai left a comment

gregorycoppola left a comment

blockstack-devops commented Nov 19, 2024

Refactor: merge block assembly paths for follower and miner #2946

Refactor: merge block assembly paths for follower and miner #2946

Conversation

pavitthrap commented Nov 22, 2021 • edited Loading

codecov bot commented Nov 22, 2021 • edited Loading

Codecov Report

kantai commented Nov 23, 2021 • edited Loading

jcnelson Nov 29, 2021

Choose a reason for hiding this comment

gregorycoppola Nov 30, 2021

Choose a reason for hiding this comment

kantai Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

gregorycoppola Dec 1, 2021

Choose a reason for hiding this comment

jcnelson Dec 7, 2021

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

gregorycoppola Nov 30, 2021

Choose a reason for hiding this comment

jcnelson Dec 7, 2021

Choose a reason for hiding this comment

gregorycoppola Nov 30, 2021

Choose a reason for hiding this comment

gregorycoppola commented Dec 1, 2021

gregorycoppola commented Dec 1, 2021

kantai commented Dec 1, 2021

kantai left a comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 19, 2024

pavitthrap commented Nov 22, 2021 •

edited

Loading

codecov bot commented Nov 22, 2021 •

edited

Loading

kantai commented Nov 23, 2021 •

edited

Loading

kantai Dec 1, 2021 •

edited

Loading