fix: Fix the GasEstimateMessageGas API
#6109
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughFlip message-filter gate in chain store, add default max-fee config and thread it into ChainConfig, make gas estimation tipset-aware with median premium and fee capping, change RPC message responses to include CID (FlattenedApiMessage), update wallet/tests to use new shapes and add GasEstimateMessageGas test coverage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RPC as RPC: GasEstimateMessageGas
participant Gas as rpc/methods/gas.rs
participant Chain as ChainStore
participant Cfg as ChainConfig
Client->>RPC: request(msg, msg_spec, tsk)
RPC->>Gas: estimate_message_gas(msg, msg_spec, tsk)
Gas->>Chain: load_required_tipset_or_heaviest(tsk)
Chain-->>Gas: Tipset
Gas->>Gas: collect recent prices/blocks
Gas->>Gas: median_gas_premium_calculation()
Gas->>Cfg: read default_max_fee
Cfg-->>Gas: Option<TokenAmount>
Gas->>Gas: cap_gas_fee(default_max_fee, msg, msg_spec)
Gas-->>RPC: FlattenedApiMessage { message, cid }
RPC-->>Client: FlattenedApiMessage (JSON flattened, CID as "CID")
sequenceDiagram
autonumber
actor User
participant CLI as cli/config.rs
participant Daemon as daemon/context.rs
participant Net as networks/mod.rs
User->>CLI: load config (includes fee.max_fee)
CLI-->>Daemon: Config
Daemon->>Net: ChainConfig::new(...)
Net-->>Daemon: ChainConfig (default_max_fee: None)
Daemon->>Daemon: set ChainConfig.default_max_fee = Some(config.fee.max_fee)
Daemon-->>User: Node started with default_max_fee available to RPC
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Comment |
akaladarshi
requested review from
elmattic and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/chain/store/chain_store.rs (3)
655-661: Correct first-encounter initialization for sender — good fixInitializing sequence/balance on first sight of a sender prevents prematurely skipping valid messages. This aligns with Lotus behavior.
You can reduce lookups with an entry-based pattern to initialize both maps once and avoid the extra contains/get_mut passes.
656-661: Hard error on missing actor may be too strictIf
state.get_actor(from_address)?returnsNone, this path errors the whole call. In practice, a missing actor for a message is unexpected, but if encountered, failing the entire RPC may be harsh. Consider skipping such messages with a warn to keep the endpoint resilient.Proposed change:
- let actor_state = state - .get_actor(from_address)? - .ok_or_else(|| Error::Other("Actor state not found".to_string()))?; - applied.insert(*from_address, actor_state.sequence); - balances.insert(*from_address, actor_state.balance.clone().into()); + match state.get_actor(from_address)? { + Some(actor_state) => { + applied.insert(*from_address, actor_state.sequence); + balances.insert(*from_address, actor_state.balance.clone().into()); + } + None => { + tracing::warn!("messages_for_tipset: actor not found for {}", from_address); + continue; + } + }
662-677: Remove unreachable else branches after guaranteed initializationAfter inserting the sender into
applied/balancesabove, theelse { continue; }branches are effectively unreachable. Removing them simplifies flow and reduces cognitive load.- if let Some(seq) = applied.get_mut(from_address) { + if let Some(seq) = applied.get_mut(from_address) { if *seq != message.sequence() { continue; } *seq += 1; - } else { - continue; - } + } if let Some(bal) = balances.get_mut(from_address) { if *bal < message.required_funds() { continue; } *bal -= message.required_funds(); - } else { - continue; - } + }src/lotus_json/message.rs (1)
75-101: Update snapshots/tests to account for new CID field in JSON
into_lotus_jsonnow populatescid: Some(self.cid()), so serialized JSON will include"CID". The hard-coded JSON insnapshots()omits it and will likely fail.
- Update the snapshot JSON to include the expected
"CID"field for the default message, or- Relax the snapshot comparison to ignore
"CID"if that better fits the test intent.Also applies to: 116-117
src/rpc/methods/gas.rs (2)
165-168: Noise multiplication introduces a small positive biasAdding
+ 1f64inside the scaled noise factor biases results upward by ~1 ULP. You can remove it while keeping the same fixed‑point scaling.- premium *= BigInt::from_f64((noise * (1i64 << precision) as f64) + 1f64) + let scale = 1i64 << precision; + premium *= BigInt::from_f64(noise * scale as f64) .context("failed to convert gas premium f64 to bigint")?; - premium = premium.div_floor(1i64 << precision); + premium = premium.div_floor(scale);
354-385: Guard against division by zero when capping by MaxFeeAlthough
estimate_message_gassets a nonzero gas limit earlier, adding a defensive check keepscap_gas_feesafe if reused elsewhere.- let gas_limit = msg.gas_limit(); - let total_fee = msg.gas_fee_cap() * gas_limit; + let gas_limit = msg.gas_limit(); + if gas_limit == 0 { + return; + } + let total_fee = msg.gas_fee_cap() * gas_limit;src/rpc/methods/eth.rs (2)
829-829: Pass ApiTipsetKey(None) as a reference instead of cloningThe
estimate_gas_premiumfunction expects&ApiTipsetKey, but you're passing&ApiTipsetKey(None)which creates a temporary value. Consider creating the value first or adjusting the function signature.- let tip = crate::rpc::gas::estimate_gas_premium(&ctx, 0, &ApiTipsetKey(None)) + let ts_key = ApiTipsetKey(None); + let tip = crate::rpc::gas::estimate_gas_premium(&ctx, 0, &ts_key)
2289-2289: Consider reusing the ApiTipsetKey valueSimilar to line 829, you're creating a temporary
ApiTipsetKey(None)here as well.- match crate::rpc::gas::estimate_gas_premium(&ctx, 0, &ApiTipsetKey(None)).await { + let ts_key = ApiTipsetKey(None); + match crate::rpc::gas::estimate_gas_premium(&ctx, 0, &ts_key).await {src/cli_shared/cli/config.rs (1)
106-109: Verify the default max fee value matches LotusThe default max fee of 0.07 FIL is correctly sourced from Lotus v1.34.1. However, using string parsing for the BigInt could be replaced with a more type-safe approach.
- max_fee: TokenAmount::from_atto( - num_bigint::BigInt::from_str("70000000000000000").unwrap(), - ), // 0.07 FIL + // 0.07 FIL = 70000000000000000 attoFIL + max_fee: TokenAmount::from_atto(70_000_000_000_000_000_u128),src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2014-2033: Consider extracting the tolerance comparison to a reusable functionThe
within_boundshelper function could be useful for other similar comparisons. Consider extracting it to a utility module if similar tolerance-based comparisons are needed elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/chain/store/chain_store.rs(1 hunks)src/cli_shared/cli/config.rs(3 hunks)src/daemon/context.rs(1 hunks)src/lotus_json/message.rs(4 hunks)src/networks/mod.rs(5 hunks)src/rpc/methods/eth.rs(2 hunks)src/rpc/methods/gas.rs(7 hunks)src/rpc/types/mod.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
🧬 Code graph analysis (5)
src/cli_shared/cli/config.rs (2)
src/utils/misc/env.rs (1)
is_env_set_and_truthy(23-27)src/shim/econ.rs (1)
from_atto(106-108)
src/lotus_json/message.rs (1)
src/blocks/tipset.rs (1)
cid(40-48)
src/rpc/methods/eth.rs (1)
src/rpc/methods/gas.rs (1)
estimate_gas_premium(108-170)
src/tool/subcommands/api_cmd/api_compare_tests.rs (3)
src/rpc/reflect/mod.rs (1)
request(250-260)src/blocks/tipset.rs (2)
key(336-339)key(530-533)src/shim/econ.rs (2)
is_zero(68-70)div_floor(130-132)
src/rpc/methods/gas.rs (3)
src/rpc/types/tsk_impl.rs (1)
ts(31-31)src/shim/econ.rs (2)
from_atto(106-108)zero(65-67)src/message/chain_message.rs (1)
gas_limit(83-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
🔇 Additional comments (14)
src/lotus_json/message.rs (1)
43-50: Expose CID in Lotus JSON while ignoring inbound CID — good balanceIncluding
CIDin the JSON representation while ignoring the inbound field on deserialization avoids spoofing risks and matches Lotus’ shape.src/rpc/methods/gas.rs (7)
49-58: Tipset scoping via ApiTipsetKey — goodLoading the tipset via
load_required_tipset_or_heaviest(ts_key)(rather than always using heaviest) makes the estimate deterministic for the caller’s scope.
66-70: FeeCap includes premium when provided — goodAdding the (nonzero) gas premium to the fee cap matches Lotus’ approach.
120-123: Use parent tipsets for premium history — goodWalking parent tipsets for samples matches the intended look-back behavior.
172-198: Percentile-based premium calculation looks solidSorting by price desc and consuming gas until the 55th percentile target is reached matches the intended negative pressure behavior. Tests cover key scenarios well.
92-99: Ignoring sender and gasLimit in GasEstimateGasPremium: verify spec parityLotus’ endpoint historically ignores these params, but please confirm this remains true to avoid divergence.
387-548: Unit tests add meaningful coverageGood coverage of edge cases and multi-block behavior for the percentile algorithm.
148-156: Minimum premium floor: confirm alignment with LotusVerify MIN_GAS_PREMIUM and the tiered multipliers (nblocksincl: 1 → 2×, 2 → 1.5×, otherwise 1×) match Lotus’ GasEstimateGasPremium constants and fallback logic.
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
92-93: Add RPC snapshot for GasEstimateMessageGas — goodNew snapshot entry aligns with the added conformance coverage.
Please verify the snapshot is present in the DO bucket indexed by this manifest; otherwise CI will fail to fetch it.
src/networks/mod.rs (2)
274-275: Add default_max_fee to ChainConfig — good extensionOptional field with serde default preserves backward compatibility for existing configs. Callers can now pass a default fee (e.g., 0.07 FIL) through daemon/context.
308-309: Explicit None in constructors — consistent with serde defaultLeaving network constructors at
Noneand letting the daemon override from CLI config is a clean boundary.Also applies to: 345-346, 372-373, 405-406
src/daemon/context.rs (1)
82-82: LGTM! Proper propagation of max fee configurationThe change correctly propagates the fee configuration from the CLI config to the chain configuration. This ensures the default max fee is available where needed in the gas estimation logic.
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1994-2037: Good addition of cross-endpoint validationThe new test for
GasEstimateMessageGaswith custom validation logic is well implemented. The 5% tolerance for gas fee cap and premium accounts for the inherent non-determinism in gas estimation while still ensuring reasonable bounds.src/rpc/types/mod.rs (1)
53-55: Confirm & document breaking API: MessageSendSpec fields now publicmax_fee and maximize_fee_cap in src/rpc/types/mod.rs (MessageSendSpec) are now public — this is a breaking API change; confirm it's intentional. If intentional, add a major release-note, update API docs/changelog, and notify clients about the new maximize_fee_cap field and its impact on gas-estimation requests.
Pull request was converted to draft
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/gas.rs (1)
333-346: Guard against negative gas-limit estimates producing huge u64 via castIf
GasEstimateGasLimitreturns-1,(gl as u64)will wrap to a very large value before min-capping, which is misleading. Fail early or clamp to zero.- if msg.gas_limit == 0 { - let gl = GasEstimateGasLimit::estimate_gas_limit(data, msg.clone(), &tsk).await?; - let gl = gl as f64 * data.mpool.config.gas_limit_overestimation; - msg.set_gas_limit((gl as u64).min(BLOCK_GAS_LIMIT)); - } + if msg.gas_limit == 0 { + let gl = GasEstimateGasLimit::estimate_gas_limit(data, msg.clone(), &tsk).await?; + anyhow::ensure!(gl > 0, "gas estimation returned non-positive gas limit: {}", gl); + let gl = (gl as f64) * data.mpool.config.gas_limit_overestimation; + msg.set_gas_limit((gl as u64).min(BLOCK_GAS_LIMIT)); + }src/rpc/methods/chain.rs (1)
139-156: Return the requested CID for signed messages
Replacemessage.cid()with the inputmessage_cidso the API response CID matches the original signed message’s CID (as Lotus’s ChainGetMessage does).
🧹 Nitpick comments (3)
src/cli_shared/cli/config.rs (1)
4-4: Avoid FromStr+unwrap for the 0.07 FIL default; use a typed literalEliminate the parse+unwrap and the FromStr import by using a u128 literal. Clearer and panic-free.
- use std::str::FromStr; + // (no longer needed) impl Default for FeeConfig { fn default() -> Self { // This indicates the default max fee for a message, // The code is taken from https://github.com/filecoin-project/lotus/blob/release/v1.34.1/node/config/def.go#L39 Self { - max_fee: TokenAmount::from_atto( - num_bigint::BigInt::from_str("70000000000000000").unwrap(), - ), // 0.07 FIL + // 0.07 FIL in atto + max_fee: TokenAmount::from_atto(70_000_000_000_000_000u128), } } }Also applies to: 8-8, 13-13, 103-113
src/rpc/methods/gas.rs (2)
92-100: Gas premium refactor: good, with a small noise-factor nit
- Collecting from parent tipsets and using the new median helper improves clarity.
- Minor nit: the noise factor currently multiplies by
(noise * 2^precision + 1) / 2^precision, which effectively scales by ~noise + 2^-32. If the intent is to scale bynoisecentered at 1.0, drop the+ 1f64.- premium *= BigInt::from_f64((noise * (1i64 << precision) as f64) + 1f64) - .context("failed to convert gas premium f64 to bigint")?; - premium = premium.div_floor(1i64 << precision); + let factor = BigInt::from_f64(noise * ((1u128 << precision) as f64)) + .context("failed to convert gas premium f64 to bigint")?; + premium *= factor; + premium = premium.div_floor(1u128 << precision);Also applies to: 108-123, 148-169
347-387: Cap function: add zero gas-limit guard and consider adding testsDivision by gas_limit can panic if somehow zero leaks through. Defensive early-return is cheap insurance. Also, tests around default_max_fee and maximize_fee_cap behavior would be valuable.
fn cap_gas_fee( default_max_fee: Option<&TokenAmount>, msg: &mut Message, msg_spec: Option<MessageSendSpec>, ) { + if msg.gas_limit() == 0 { + // Nothing to cap against; leave as-is. + return; + }I can add unit tests covering:
- default_max_fee applied when spec.max_fee is zero.
- maximize_fee_cap=true behavior.
- premium clamped to feecap.
Do you want me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/cli_shared/cli/config.rs(3 hunks)src/lotus_json/signed_message.rs(1 hunks)src/rpc/methods/chain.rs(3 hunks)src/rpc/methods/eth.rs(2 hunks)src/rpc/methods/gas.rs(9 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(4 hunks)src/wallet/subcommands/wallet_cmd.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lotus_json/signed_message.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/methods/eth.rs
- src/tool/subcommands/api_cmd/api_compare_tests.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
🧬 Code graph analysis (3)
src/rpc/methods/chain.rs (2)
src/message/signed_message.rs (2)
message(40-42)cid(78-85)src/message/chain_message.rs (2)
message(22-27)cid(29-34)
src/cli_shared/cli/config.rs (1)
src/shim/econ.rs (1)
from_atto(106-108)
src/rpc/methods/gas.rs (2)
src/rpc/methods/state.rs (16)
handle(109-114)handle(132-138)handle(151-157)handle(172-178)handle(196-205)handle(223-231)handle(248-255)handle(272-282)handle(298-305)handle(335-465)handle(484-492)handle(508-538)handle(555-561)handle(577-597)handle(615-624)handle(641-663)src/message/chain_message.rs (2)
message(22-27)cid(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (7)
src/cli_shared/cli/config.rs (1)
96-101: New fee config looks goodPublic FeeConfig with lotus_json on max_fee and threading into Config is aligned with the PR goals.
Also applies to: 127-129
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
9-10: Manifest updates acknowledged; ensure objects exist in the snapshot bucketThe replacements and the added Filecoin.GasEstimateMessageGas snapshot entry look consistent with the API changes. Please verify the listed keys exist in the DO bucket to avoid CI flakiness during snapshot fetch.
Also applies to: 52-52, 72-72, 91-93
src/rpc/methods/chain.rs (1)
1040-1051: FlattenedApiMessage definition is correctSerde flatten for Message and renamed "CID" field match Lotus JSON shape. Good reuse across endpoints.
src/wallet/subcommands/wallet_cmd.rs (1)
519-521: LGTM: adjust to FlattenedApiMessageBinding to
.messageafterGasEstimateMessageGasaligns with the new return type.src/rpc/methods/gas.rs (3)
45-46: Tipset-aware fee cap estimation is a solid fixSwitching to
load_required_tipset_or_heaviest(ts_key)and threadingApiTipsetKeyremoves heaviest-tipset assumptions. Return formatting matches prior behavior.Also applies to: 49-58
172-199: Median premium helper: clear and Lotus-alignedSorting by price desc and using the 55th percentile threshold with gas-limit accumulation mirrors the Lotus approach. Unit tests cover key cases.
311-321: Return type change to FlattenedApiMessage is consistent with ChainGetMessageEncapsulating message + CID standardizes responses across RPCs. Good.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rpc/methods/chain.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/rpc/methods/chain.rs (3)
src/message/chain_message.rs (2)
message(22-27)cid(29-34)src/message/signed_message.rs (2)
message(40-42)cid(78-85)src/shim/message.rs (1)
cid(195-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
|
@akaladarshi As a wise guy once said (some Polish dude, I think),
|
akaladarshi
force-pushed
the
akaladarshi/fix-estimate-msg-gas
branch
from
83f924a to
1615714
Compare
2 participants
Summary of changes
Changes introduced in this pull request:
MaxFee0.07 FILReference issue to close (if applicable)
Closes #5940
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests