-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
propose system transactions #10971
propose system transactions #10971
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
consensus/src/round_manager.rs
Outdated
); | ||
} | ||
|
||
let (num_sys_txns, sys_txns_total_bytes) = { |
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.
nit:
proposal.sys_txns().map_or(...)
and for the closure you can use reduce
.
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.
address the comment?
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.
fixed. i thought nit is fine xD
consensus/src/round_manager_test.rs
Outdated
@@ -261,6 +261,8 @@ impl NodeSetup { | |||
PipelineBackpressureConfig::new_no_backoff(), | |||
ChainHealthBackoffConfig::new_no_backoff(), | |||
false, | |||
vec![], |
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.
should have some test coverage when it's enabled?
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.
sure. how do we want to do it: forking every test here with sys_txn_enabled = true
?
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.
prop test?
testing everything with true is probably also ok.
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.
@grao1991 how to run prop tests? any example?
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.
consensus/src/epoch_manager.rs
Outdated
@@ -132,6 +133,7 @@ pub struct EpochManager<P: OnChainConfigProvider> { | |||
commit_state_computer: Arc<dyn StateComputer>, | |||
storage: Arc<dyn PersistentLivenessStorage>, | |||
safety_rules_manager: SafetyRulesManager, | |||
sys_txn_providers: Vec<Arc<dyn SysTxnProvider>>, |
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.
you probably don't need shared ownership here?
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.
I'm not sure. But something might be easier if it's shared ownership.
E.g., at the beginning of every epoch, we need to send a clone of this vec from epoch manager to the newly created proposal manager.
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.
I think you can either send a &, or just let proposal manager owns it?
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.
epochmgr and roundmgr (owner of proposal manager) each has their own thread, I don't know how to send a & here... maybe you can suggest a sub-PR :)
move it into roundmgr (then proposal manager) is possible, but then epochmgr needs to take it back before shutting down roundmgr... doesn't sound simpler than cloning Arcs?
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.
Arc is what we've been doing unless we can figure out how to convince compiler the lifetime of round manager is shorter than epoch manager
@@ -93,6 +93,7 @@ pub enum FeatureFlag { | |||
VMBinaryFormatV7, | |||
ResourceGroupsChargeAsSizeSum, | |||
CommissionChangeDelegationPool, | |||
ProposeSystemTransactions, |
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.
Is it better to put in OnChainConsensusConfig instead? @zekun000
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.
FeatureFlag makes more sense to me. It is not consensus specific feature.
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.
tbh this is more like a consensus feature whether to propose system txn
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.
No strong opinions
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -633,10 +634,27 @@ impl RoundManager { | |||
.author() | |||
.expect("Proposal should be verified having an author"); | |||
|
|||
if !self.onchain_config.should_propose_system_txns() |
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.
we should add unit test for the new behavior
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
* propose system transaction, initial * update feature name * fmt * update * update * format update * updapte * fmt * filter out pending sys txns * UTs; resolve comments * comments * sys txn pool * update * feature flag -> consensus config * remove feature flag * nitfix * dummytopic as test-only * bugfix * update existing tests * uts for receiving limits
Description
This PR introduces a new feature
ProposeSystemTransactions
: when enabled, validator should do the following:ProposalExt
and includeSystemTransaction
s from a list ofSystemTransactionProviders
it was given.system_txn_provider[0] > ... > system_txn_provider[-1] > payload
.txn_count_limit
andblock_size_limit
.ProposalExt
.TODOs