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

[Fix] Selection of transition ID in finalize. #2575

Closed
Closed
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ jobs:
resource_class: << pipeline.parameters.twoxlarge >>
steps:
- run_serial:
flags: --test '*' -- --test-threads=8
flags: --test '*' --features test -- --test-threads=8
workspace_member: synthesizer
cache_key: v1.0.0-rust-1.81.0-snarkvm-synthesizer-integration-cache

Expand Down
10 changes: 8 additions & 2 deletions console/network/src/canary_v0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,16 @@ impl Network for CanaryV0 {
/// The block height from which consensus V2 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V2_HEIGHT: u32 = 2_900_000;
// TODO (raychu86): Update this value based on the desired canary height.
/// The block height from which consensus V2 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V2_HEIGHT: u32 = 0;
const CONSENSUS_V2_HEIGHT: u32 = 10;
// TODO: Update this value based on the desired canary height.
// The block height from which consensus V3 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V3_HEIGHT: u32 = 3_900_000;
/// The block height from which consensus V3 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V3_HEIGHT: u32 = 11;
/// The network edition.
const EDITION: u16 = 0;
/// The genesis block coinbase target.
Expand Down
2 changes: 2 additions & 0 deletions console/network/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ pub trait Network:

/// The block height from which consensus V2 rules apply.
const CONSENSUS_V2_HEIGHT: u32;
/// The block height from which consensus V3 rules apply.
const CONSENSUS_V3_HEIGHT: u32;

/// The function name for the inclusion circuit.
const INCLUSION_FUNCTION_NAME: &'static str;
Expand Down
8 changes: 7 additions & 1 deletion console/network/src/mainnet_v0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,16 @@ impl Network for MainnetV0 {
/// The block height from which consensus V2 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V2_HEIGHT: u32 = 2_800_000;
// TODO (raychu86): Update this value based on the desired mainnet height.
/// The block height from which consensus V2 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V2_HEIGHT: u32 = 10;
// TODO: Update this value based on the desired mainnet height.
/// The block height from which consensus V3 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V3_HEIGHT: u32 = 3_800_000;
/// The block height from which consensus V3 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V3_HEIGHT: u32 = 11;
/// The network edition.
const EDITION: u16 = 0;
/// The genesis block coinbase target.
Expand Down
8 changes: 7 additions & 1 deletion console/network/src/testnet_v0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,16 @@ impl Network for TestnetV0 {
/// The block height from which consensus V2 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V2_HEIGHT: u32 = 2_950_000;
// TODO (raychu86): Update this value based on the desired testnet height.
/// The block height from which consensus V2 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V2_HEIGHT: u32 = 10;
// TODO: Update this value based on the desired testnet height.
/// The block height from which consensus V3 rules apply.
#[cfg(not(any(test, feature = "test")))]
const CONSENSUS_V3_HEIGHT: u32 = 3_950_000;
/// The block height from which consensus V3 rules apply.
#[cfg(any(test, feature = "test"))]
const CONSENSUS_V3_HEIGHT: u32 = 11;
/// The network edition.
const EDITION: u16 = 0;
/// The genesis block coinbase target.
Expand Down
2 changes: 1 addition & 1 deletion synthesizer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ serial = [
"synthesizer-snark/serial"
]
setup = [ ]
test = [ "console/test" ]
test = [ "console/test", "ledger-block/test", "ledger-store/test" ]
timer = [ "aleo-std/timer" ]
wasm = [
"process",
Expand Down
54 changes: 38 additions & 16 deletions synthesizer/process/src/finalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ impl<N: Network> Process<N> {
lap!(timer, "Verify the number of transitions");

// Construct the call graph.
let call_graph = self.construct_call_graph(execution)?;
let call_graph = if state.block_height() >= N::CONSENSUS_V3_HEIGHT {
Default::default()
d0cd marked this conversation as resolved.
Show resolved Hide resolved
} else {
self.construct_call_graph(execution)?
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit tangential to what's being fixed, but my fear is this approach of a CONSENSUS_VX_HEIGHT is quickly creating a situation where we could have double digit consensus versions and migratory logic like this in many places within the the SnarkVM codebase. This could easily lead to nontrivial errors.

At some point soon, the community should come up with some kind of migration model and spec which lays out a standard format for migration.

I haven't thought this out in enough depth yet, but I imagine we might have some kind of migration decorator-style pattern (perhaps a macro? Enums that apply migration functions, etc.?) that clearly denotes a change in consensus rules, specifies the logic at each and does a standard set of checks.

After the higher priority fixes mainnet the community should do some active design to keep future updates and fixes maintainable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a good call out.
Your macro suggestion reminded me of the malice approach, maybe we can borrow the design there.
We could also enforce stricter standards around migration logic, requiring that a README is maintained in this repo, which defines the invariants around each new CONSENSUS_V*_HEIGHT.

Copy link
Collaborator

@vicsn vicsn Nov 22, 2024

Choose a reason for hiding this comment

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

+1 very much something worth thinking about and documenting.

One design principle I'm confident about (which also went wrong in the first design of the Fee lowering PR) is that wherever consensus changes are introduced, they should be independent full self-contained functionality, and not complex derivations of each other's functionality.

A second one: whether we should always use < or >= or a match for the comparison.

Copy link
Contributor

@raychu86 raychu86 Dec 5, 2024

Choose a reason for hiding this comment

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

@iamalwaysuncomfortable this is a wonderful point. Thank you for bringing it up.

I also have a great dislike for CONSENSUS_VX_HEIGHT as it will quickly grow out of control. It's definitely worth exploring cleaner solutions. There should be plenty of examples of other chains that have managed quick bug fixes, hard forks, etc. that we can learn from.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Provable protocol team will hereby promise to do a small lit review in january.


atomic_batch_scope!(store, {
// Finalize the root transition.
Expand Down Expand Up @@ -160,9 +164,11 @@ fn finalize_fee_transition<N: Network, P: FinalizeStorage<N>>(
fee: &Fee<N>,
) -> Result<Vec<FinalizeOperation<N>>> {
// Construct the call graph.
let mut call_graph = HashMap::new();
// Insert the fee transition.
call_graph.insert(*fee.transition_id(), Vec::new());
let call_graph = if state.block_height() >= N::CONSENSUS_V3_HEIGHT {
d0cd marked this conversation as resolved.
Show resolved Hide resolved
Default::default()
d0cd marked this conversation as resolved.
Show resolved Hide resolved
} else {
HashMap::from([(*fee.transition_id(), Vec::new())])
};

// Finalize the transition.
match finalize_transition(state, store, stack, fee, call_graph) {
Expand Down Expand Up @@ -208,8 +214,11 @@ fn finalize_transition<N: Network, P: FinalizeStorage<N>>(
// Initialize a stack of active finalize states.
let mut states = Vec::new();

// Initialize a nonce for the finalize registers.
let mut nonce = 0;

// Initialize the top-level finalize state.
states.push(initialize_finalize_state(state, future, stack, *transition.id())?);
states.push(initialize_finalize_state(state, future, stack, *transition.id(), nonce)?);

// While there are active finalize states, finalize them.
'outer: while let Some(FinalizeState {
Expand Down Expand Up @@ -263,20 +272,30 @@ fn finalize_transition<N: Network, P: FinalizeStorage<N>>(
await_.register()
);

// Get the current transition ID.
let transition_id = registers.transition_id();
// Get the child transition ID.
let child_transition_id = match call_graph.get(transition_id) {
Some(transitions) => match transitions.get(call_counter) {
Some(transition_id) => *transition_id,
None => bail!("Child transition ID not found."),
},
None => bail!("Transition ID '{transition_id}' not found in call graph"),
// Get the transition ID used to initialize the finalize registers.
// If the block height is greater than or equal to the consensus V3 height, return the main transition ID.
// Otherwise, query the call graph for the child transition ID corresponding to the future that is being awaited.
let transition_id = if state.block_height() >= N::CONSENSUS_V3_HEIGHT {
*transition.id()
} else {
// Get the current transition ID.
let transition_id = registers.transition_id();
// Get the child transition ID.
match call_graph.get(transition_id) {
Some(transitions) => match transitions.get(call_counter) {
Some(transition_id) => *transition_id,
None => bail!("Child transition ID not found."),
},
None => bail!("Transition ID '{transition_id}' not found in call graph"),
}
};

// Increment the nonce.
nonce += 1;
d0cd marked this conversation as resolved.
Show resolved Hide resolved

// Set up the finalize state for the await.
let callee_state =
match try_vm_runtime!(|| setup_await(state, await_, stack, &registers, child_transition_id)) {
match try_vm_runtime!(|| setup_await(state, await_, stack, &registers, transition_id, nonce)) {
Ok(Ok(callee_state)) => callee_state,
// If the evaluation fails, bail and return the error.
Ok(Err(error)) => bail!("'finalize' failed to evaluate command ({command}): {error}"),
Expand Down Expand Up @@ -357,6 +376,7 @@ fn initialize_finalize_state<'a, N: Network>(
future: &Future<N>,
stack: &'a Stack<N>,
transition_id: N::TransitionID,
nonce: u64,
) -> Result<FinalizeState<'a, N>> {
// Get the finalize logic and the stack.
let (finalize, stack) = match stack.program_id() == future.program_id() {
Expand All @@ -381,6 +401,7 @@ fn initialize_finalize_state<'a, N: Network>(
transition_id,
*future.function_name(),
stack.get_finalize_types(future.function_name())?.clone(),
nonce,
);

// Store the inputs.
Expand All @@ -402,14 +423,15 @@ fn setup_await<'a, N: Network>(
stack: &'a Stack<N>,
registers: &FinalizeRegisters<N>,
transition_id: N::TransitionID,
nonce: u64,
) -> Result<FinalizeState<'a, N>> {
// Retrieve the input as a future.
let future = match registers.load(stack, &Operand::Register(await_.register().clone()))? {
Value::Future(future) => future,
_ => bail!("The input to 'await' is not a future"),
};
// Initialize the state.
initialize_finalize_state(state, &future, stack, transition_id)
initialize_finalize_state(state, &future, stack, transition_id, nonce)
}

// A helper function that returns the index to branch to.
Expand Down
19 changes: 18 additions & 1 deletion synthesizer/process/src/stack/finalize_registers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ pub struct FinalizeRegisters<N: Network> {
finalize_types: FinalizeTypes<N>,
/// The mapping of assigned registers to their values.
registers: IndexMap<u64, Value<N>>,
/// A nonce for finalize registers.
nonce: u64,
/// The tracker for the last register locator.
last_register: Option<u64>,
}
Expand All @@ -58,8 +60,17 @@ impl<N: Network> FinalizeRegisters<N> {
transition_id: N::TransitionID,
function_name: Identifier<N>,
finalize_types: FinalizeTypes<N>,
nonce: u64,
) -> Self {
Self { state, transition_id, finalize_types, function_name, registers: IndexMap::new(), last_register: None }
Self {
state,
transition_id,
finalize_types,
function_name,
registers: IndexMap::new(),
nonce,
last_register: None,
}
}
}

Expand All @@ -81,4 +92,10 @@ impl<N: Network> FinalizeRegistersState<N> for FinalizeRegisters<N> {
fn function_name(&self) -> &Identifier<N> {
&self.function_name
}

/// Returns the nonce for the finalize registers.
#[inline]
fn nonce(&self) -> u64 {
self.nonce
}
}
2 changes: 1 addition & 1 deletion synthesizer/process/src/stack/register_types/initialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl<N: Network> RegisterTypes<N> {
}

/* Additional checks. */
// - All futures produces before the `async` call must be consumed by the `async` call.
// - All futures produced before the `async` call must be consumed by the `async` call.

// Get all registers containing futures.
let mut future_registers: IndexSet<(Register<N>, Locator<N>)> = register_types
Expand Down
31 changes: 22 additions & 9 deletions synthesizer/program/src/logic/command/rand_chacha.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,28 @@ impl<N: Network> RandChaCha<N> {
let seeds: Vec<_> = self.operands.iter().map(|operand| registers.load(stack, operand)).try_collect()?;

// Construct the random seed.
let preimage = to_bits_le![
registers.state().random_seed(),
**registers.transition_id(),
stack.program_id(),
registers.function_name(),
self.destination.locator(),
self.destination_type.type_id(),
seeds
];
let preimage = if registers.state().block_height() >= N::CONSENSUS_V3_HEIGHT {
d0cd marked this conversation as resolved.
Show resolved Hide resolved
to_bits_le![
registers.state().random_seed(),
**registers.transition_id(),
stack.program_id(),
registers.function_name(),
registers.nonce(),
self.destination.locator(),
self.destination_type.type_id(),
seeds
]
} else {
to_bits_le![
registers.state().random_seed(),
**registers.transition_id(),
stack.program_id(),
registers.function_name(),
self.destination.locator(),
self.destination_type.type_id(),
seeds
]
};

// Hash the preimage.
let digest = N::hash_bhp1024(&preimage)?.to_bytes_le()?;
Expand Down
3 changes: 3 additions & 0 deletions synthesizer/program/src/traits/stack_and_registers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ pub trait FinalizeRegistersState<N: Network> {

/// Returns the function name for the finalize scope.
fn function_name(&self) -> &Identifier<N>;

/// Returns the nonce for the finalize registers.
fn nonce(&self) -> u64;
}

pub trait RegistersSigner<N: Network> {
Expand Down
1 change: 1 addition & 0 deletions synthesizer/program/tests/helpers/sample.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ pub fn sample_finalize_registers(
<CurrentNetwork as Network>::TransitionID::default(),
*function_name,
stack.get_finalize_types(function_name)?.clone(),
0u64,
);

// For each literal,
Expand Down
Loading