Conversation
karlem
force-pushed
the
f3-proofs-cache
branch
from
fbaa095 to
b34142d
Compare
karlem
force-pushed
the
f3-proofs-cache
branch
from
b34142d to
31feb85
Compare
karlem
force-pushed
the
f3-lifecycle
branch
2 times, most recently
from
91db005 to
cbce51c
Compare
karlem
force-pushed
the
f3-proofs-cache
branch
from
39e59d6 to
bfdc6f7
Compare
karlem
force-pushed
the
f3-proofs-cache
branch
from
93a1066 to
1747f78
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // | ||
| // Note: the first epoch proven in a certificate does not have a previous cursor. |
There was a problem hiding this comment.
We should be able to initialize the cursor from the base tipset's storage commitment (used only as parent here), which is the same as the last tipset in the previous cert (used as child there). WDYT?
There was a problem hiding this comment.
As a workaround, maybe we could fetch parent tipsets (which are not very useful anyway) right in generate_proof_for_epoch, call it for the base tipset, get the nonces from the resulting proof bundle and discard the proof. Perhaps, we can do this only once and then maintain the cursor in ProofGeneratorService struct.
There was a problem hiding this comment.
I think the best way here is to store it on L2 ledger...
sergefdrv
left a comment
sergefdrv
left a comment
There was a problem hiding this comment.
I think it should work, so we can test it against calibration and finally get merged 🚀 Good job! 💪
| verify_sequence_against_storage_next( | ||
| "top-down message nonces", | ||
| next_topdown, | ||
| cursor.as_ref().map(|c| c.next_topdown_message_nonce), | ||
| Some(cursor.next_parent_topdown_nonce), | ||
| &nums.topdown_nonces, | ||
| )?; | ||
| verify_sequence_against_storage_next( | ||
| "power-change configuration numbers", | ||
| next_cfg, | ||
| cursor.as_ref().map(|c| c.next_power_change_config_number), | ||
| Some(cursor.next_parent_power_change_config_number), | ||
| &nums.config_numbers, | ||
| )?; |
There was a problem hiding this comment.
Maybe verify_sequence_against_storage_next could also require the nonce rather than taking it wrapped in Option.
| let start = Instant::now(); | ||
| let (state, (apply_ret, emitters)) = state.call(*msg.clone()).await?; | ||
| let latency = start.elapsed().as_secs_f64(); | ||
| let exit_code = apply_ret.msg_receipt.exit_code.value(); | ||
| emit(MsgExec { | ||
| purpose: MsgExecPurpose::Call, | ||
| height: state.block_height(), | ||
| message: *msg, | ||
| duration: latency, | ||
| duration: 0.0, |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Power table canonical ordering gets lost
- Fixed get_state to sort power table by canonical F3 ordering (power descending, then participant id ascending) instead of sorting by id only, ensuring consistency with F3 certificates and power-table CIDs.
Or push these changes by commenting:
@cursor push 7d00f19e5a
Preview (7d00f19e5a)
diff --git a/fendermint/actors/f3-light-client/src/lib.rs b/fendermint/actors/f3-light-client/src/lib.rs
--- a/fendermint/actors/f3-light-client/src/lib.rs
+++ b/fendermint/actors/f3-light-client/src/lib.rs
@@ -6,6 +6,7 @@
use fil_actors_runtime::builtin::singletons::SYSTEM_ACTOR_ADDR;
use fil_actors_runtime::runtime::{ActorCode, Runtime};
use fil_actors_runtime::{actor_dispatch, actor_error, ActorError};
+use fvm_shared::bigint::{BigInt, Sign};
use fvm_shared::METHOD_CONSTRUCTOR;
use num_derive::FromPrimitive;
@@ -103,7 +104,13 @@
});
Ok(())
})?;
- out.sort_by_key(|e| e.id);
+ // Sort by canonical F3 ordering: power descending, then participant id ascending.
+ // This matches the ordering used in F3 certificates and power-table CIDs.
+ out.sort_by(|a, b| {
+ let pa = BigInt::from_bytes_be(Sign::Plus, &a.power_be);
+ let pb = BigInt::from_bytes_be(Sign::Plus, &b.power_be);
+ pb.cmp(&pa).then_with(|| a.id.cmp(&b.id))
+ });
out
};
@@ -137,8 +144,18 @@
use fil_actors_runtime::SYSTEM_ACTOR_ADDR;
use fvm_ipld_encoding::ipld_block::IpldBlock;
use fvm_shared::address::Address;
+ use fvm_shared::bigint::{BigInt, Sign};
use fvm_shared::error::ExitCode;
+ /// Helper function to sort power entries to canonical F3 ordering
+ fn sort_power_entries_canonical(entries: &mut Vec<PowerEntry>) {
+ entries.sort_by(|a, b| {
+ let pa = BigInt::from_bytes_be(Sign::Plus, &a.power_be);
+ let pb = BigInt::from_bytes_be(Sign::Plus, &b.power_be);
+ pb.cmp(&pa).then_with(|| a.id.cmp(&b.id))
+ });
+ }
+
/// Helper function to create test power entries
fn create_test_power_entries() -> Vec<PowerEntry> {
fn u64_to_power_be(x: u64) -> Vec<u8> {
@@ -321,7 +338,9 @@
let response = result.deserialize::<GetStateResponse>().unwrap();
assert_eq!(response.processed_instance_id, 42);
- assert_eq!(response.power_table, power_entries);
+ let mut expected = power_entries;
+ sort_power_entries_canonical(&mut expected);
+ assert_eq!(response.power_table, expected);
}
#[test]
@@ -384,7 +403,9 @@
initial.power_table_root, updated.power_table_root,
"power table root CID should change when table changes"
);
- assert_eq!(updated.power_table, new_power_table);
+ let mut expected = new_power_table;
+ sort_power_entries_canonical(&mut expected);
+ assert_eq!(updated.power_table, expected);
}
#[test]This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| Ok(()) | ||
| })?; | ||
| out.sort_by_key(|e| e.id); | ||
| out |
There was a problem hiding this comment.
Power table canonical ordering gets lost
High Severity
get_state now sorts power_table by validator id, but F3 power tables are canonicalized by power descending then id ascending. Passing this reordered table into power_entries_from_actor changes table semantics and can break certificate/proof progression tied to canonical ordering.
Additional Locations (1)
| let latency = generation_start.elapsed().as_secs_f64(); | ||
|
|
||
| emit(ProofBundleGenerated { | ||
| highest_epoch: parent_epoch, | ||
| storage_proofs: bundle.storage_proofs.len(), | ||
| event_proofs: bundle.event_proofs.len(), | ||
| witness_blocks: bundle.blocks.len(), | ||
| bundle_size_bytes, | ||
| status: OperationStatus::Success, | ||
| latency, | ||
| latency: 0.0, |
| let persist_start = Instant::now(); | ||
| if let Err(e) = | ||
| self.with_persistence(|p| p.save_certificate_with_epoch_proofs(&cert, &epoch_proofs)) | ||
| { | ||
| emit(ProofCacheAtomicWrite { | ||
| instance: instance_id, | ||
| epoch_count: epochs.len(), | ||
| status: OperationStatus::Failure, | ||
| latency: persist_start.elapsed().as_secs_f64(), | ||
| latency: 0.0, | ||
| }); | ||
| return Err(e); | ||
| } | ||
| emit(ProofCacheAtomicWrite { | ||
| instance: instance_id, | ||
| epoch_count: epochs.len(), | ||
| status: OperationStatus::Success, | ||
| latency: persist_start.elapsed().as_secs_f64(), | ||
| latency: 0.0, |
| /// If cache locks are contended by writers (e.g. proof insertion), this returns `None` | ||
| /// immediately so consensus proposal building can continue without stalling. |
There was a problem hiding this comment.
Oh, this may cause starvation on the proposer side...
| let fetch_start = Instant::now(); | ||
|
|
||
| match self.light_client.get_certificate(instance).await { | ||
| Ok(cert) => { | ||
| let latency = fetch_start.elapsed().as_secs_f64(); | ||
| emit(F3CertificateFetched { | ||
| instance, | ||
| ec_chain_len: cert.ec_chain.suffix().len(), | ||
| status: OperationStatus::Success, | ||
| latency, | ||
| latency: 0.0, |
| let latency = fetch_start.elapsed().as_secs_f64(); | ||
| emit(F3CertificateFetched { | ||
| instance, | ||
| ec_chain_len: 0, | ||
| status: OperationStatus::Failure, | ||
| latency, | ||
| latency: 0.0, |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| tracing::info!(height, "PrepareProposal message preparation finished"); | ||
|
|
||
| let txs = Vec::from_iter(response.messages.into_iter().map(bytes::Bytes::from)); | ||
| tracing::debug!(height, tx_count = txs.len(), "PrepareProposal response ready"); |
There was a problem hiding this comment.
Debug-level logging promoted to info in prepare_proposal
Low Severity
Several tracing::info! calls were added to prepare_proposal ("PrepareProposal start", "read_only_view ready", "message preparation finished", "response ready") that fire on every single block proposal. Since prepare_proposal runs on every block, these produce excessive noise at info level in production. They look like development instrumentation that was left at info! instead of being downgraded to debug! or trace!.
2 participants
Closes #1441 and #1442
Note
High Risk
High risk because it changes consensus-critical top-down finality initialization/execution paths, updates on-chain actor state schema for the F3 light client, and alters genesis creation to pull/derive F3 parameters from a parent chain.
Overview
Adds an end-to-end F3 proof-based top-down finality path: new
app::service::topdownbootstraps either legacy vote-based topdown or F3 (with persistent proof cache, proof-service background task, and retry-on-cache-miss execution config), andnodestartup is refactored to use this pre/post-App::new()initialization.Reworks the
f3-light-clientactor to store the power table as a HAMT rooted bypower_table_root, trackprocessed_instance_idwith monotonicity checks, and materialize the table onGetState; updates associated types/tests and adds sharedfendermint_vm_evm_event_utilsfor decoding EVM event proofs.Extends genesis-from-parent to fetch/validate an F3 certificate, derive
base_epochand its ETH block hash, parse/sort power using big-int bytes, and adds configurable F3 proof-service + execution retry settings infendermint_app_settings(with updated example config).Written by Cursor Bugbot for commit 682a9e8. This will update automatically on new commits. Configure here.