-
Notifications
You must be signed in to change notification settings - Fork 713
Initial implementation of mockamoto stack-node mode #4001
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
Conversation
Codecov Report
@@ Coverage Diff @@
## next #4001 +/- ##
==========================================
- Coverage 0.16% 0.16% -0.01%
==========================================
Files 350 351 +1
Lines 292190 292666 +476
==========================================
Hits 469 469
- Misses 291721 292197 +476
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Does this mean the blocks include the new |
Yes, it does include the new transaction type. Once the mocknet node integrates with the chain coordinator work (which this PR doesn't yet), then yeah, the mocknet node is a good place to start integrating the new tx type. Until then, it won't emit events (events are emitted by the chain coordinator). |
|
@kantai : Is this still a draft? Can I request reviews? |
|
Yes its ready for reviews. This PR is ultimately pretty minor and will just be a good place for merging in the work from Jude's coordinator PR. |
|
Would having more reviewers help or is that not a bottleneck right now? I understand that only people currently on the Nakamoto WG are probably best for reviewing but just wondering if reviewing is something that needs more attention or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a couple of requests for added comments for clarity, but it looks great!
| let coinbase_tx = coinbase_tx_signer.get_tx().unwrap(); | ||
|
|
||
| let parent_block_id = StacksBlockId::new(&chain_tip_ch, &chain_tip_bh); | ||
| let tenure_change_tx_payload = TransactionPayload::TenureChange(TenureChangePayload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here? I guess you're just including a tenure change in every block to test some more paths?
| Ok(new_snapshot) | ||
| } | ||
|
|
||
| pub struct MockamotoNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment on this struct, or a top-level commenting explaining what Mockamoto does and why it is useful? It doesn't need to be anything to complicated, since this is just temporary, but I think this will be helpful for others using it during development.
| clarity_instance: &'a mut ClarityInstance, | ||
| sortition_dbconn: &'b dyn SortitionDBRef, | ||
| pox_constants: &PoxConstants, | ||
| chain_tip: &StacksHeaderInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand why replacing chain_tip with parent_stacks_height and parent_burn_height is required? This seems needless to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It reduces the amount that needs to be mocked: the chain_tip argument is only used for obtaining the parent stacks height and parent burn height (for epoch checks, mostly). It also makes writing unit tests easier. An argument like chain_tip which includes a bunch of information that the function doesn't use confuses the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this to move things along, but please re-consider the change to setup_block's arguments.
|
Merging -- if we want to change the |
|
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. |
This PR implements a first version of mockamoto. It has several deficiencies at the moment, but those could be sorted in separate PRs (which is, perhaps, an argument for leaving those to a separate PR):
If those don't get covered by this PR, I'll open issues for them.
This is still a draft because right now, there's no associated unit tests -- it may also be worthwhile to just include some of the above features in this PR.