[Coinbase Go/No-Go] Bond balances in genesis block #2308
Conversation
ledger/block/src/ratify/mod.rs
Outdated
|
|
||
| #[allow(clippy::large_enum_variant)] |
There was a problem hiding this comment.
I wonder if this has a non-trivial impact on object/storage size?
There was a problem hiding this comment.
It might have some impact, but the jump from (Committee<N>, PublicBalances<N>) to (Committee<N>, PublicBalances<N>, BondedBalances<N>) won't be that big.
There was a problem hiding this comment.
Considering every block contains this enum, and we will always pay the worst-case memory footprint, I don't understand why one chose to ignore this clippy warning.
You should follow the advise of clippy to resolve it (I assume its to use Box). Otherwise, every enum variant will always pay the worst case bound.
There was a problem hiding this comment.
why one chose to ignore this clippy warning
By being too much in a get shit done mood, my bad.
@d0cd since you've been finishing off the PR, want to wrap the contents in Box? I also just recently noticed we're already using this pattern in snarkVM.
There was a problem hiding this comment.
Done! By boxing the elements of Genesis::Ratify the size reduces from 240 to 32 bytes.
synthesizer/src/vm/finalize.rs
Outdated
| // Update the bonded balance in finalize storage. | ||
| let operation = store.update_key_value(program_id, bonded_mapping, key, next_value)?; | ||
| finalize_operations.push(operation); | ||
| } |
There was a problem hiding this comment.
I haven't looked into this in depth, but outside of the bonded mapping, we need to update the committee_state in the committee mapping and also the number of delegators if/when this PR gets merged in - https://github.com/AleoHQ/snarkVM/pull/2213.
In addition, we need checks that the delegated bonding amount meets all the requirements (for example, delegation must be more than X credits)
vicsn
force-pushed
the
bond_balances_in_genesis
branch
from
960a41b to
74d29bf
Compare
|
Would be nice to have some tests that ensures the bond balances is equivalent to actual |
synthesizer/src/vm/finalize.rs
Outdated
| // Check that the amount meets the minimum requirement, depending on whether the address is a validator. | ||
| if *address == *validator_address { | ||
| ensure!( | ||
| *amount >= 1_000_000_000_000_u64, |
There was a problem hiding this comment.
Is there a way to ensure that these values match the credits.aleo minimum requirements? Might be good to write some comments and maybe add some tests for enforcement to catch any changes down the line.
…_balances_in_genesis
synthesizer/src/vm/finalize.rs
Outdated
| }) | ||
| .collect_vec(); | ||
| assert_eq!(actual_account.len(), expected_account.len()); | ||
| // Check that all entries except for the one for the first validator are the same. |
There was a problem hiding this comment.
Fix comment. Also why is the first validator entry different?
| // Check that all entries except for the one for the first validator are the same. | |
| // Check that all entries except for the first validator are the same. |
There was a problem hiding this comment.
This is because the first validator also executes a number of additional transactions in the genesis_quorum method.
Consequently, its balance after adding the genesis block is not the same as the public balance provided in Genesis::Ratify.
There was a problem hiding this comment.
Could you maybe clarify that in the comment? Otherwise, we won't know why down the line.
| .collect_vec(); | ||
| assert_eq!(actual_bonded.len(), expected_bonded.len()); | ||
| for entry in actual_bonded.iter() { | ||
| assert!(expected_bonded.contains(entry)); |
There was a problem hiding this comment.
Why don't we check the amounts in the bonded and account mappings?
There was a problem hiding this comment.
NVM, i see now that the actual_bonded and expected_bonded variables are Vectors, so the contains call will check that the balance is correct.
Co-authored-by: Raymond Chu <14917648+raychu86@users.noreply.github.com> Signed-off-by: d0cd <23022326+d0cd@users.noreply.github.com>
howardwu
changed the title
Motivation
We want to be able to bond balances in the genesis block.
This PR modifies
Genesis::Ratify, such that it contains the desired state of thecommittee,account, andbondedmappings incredits.aleo. This initial state is checked for consistency before it is directly injected into theFinalizeStore.Related PRs
https://github.com/AleoHQ/snarkOS/pull/3016