Thiagodeev/refactor-add-txn#3582
Draft
thiagodeev wants to merge 15 commits intomainfrom
Draft
Conversation
… pointers in ResourceBoundsMap
… required fields and remove JSON unmarshal method
…ssary nil checks and simplifying resource bounds handling
… handling with generics
…mize AddTransaction flow In our old code, we were unnecessarily compiling the casm class in the ' h.receivedTransactionFeed != nil' condition
…create a new payload type for it
2827ddb to
f00cd36
Compare
…dcastedTransactionToCore and CompileBroadcastedDeclareTxn
f00cd36 to
9920346
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (45.09%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3582 +/- ##
==========================================
- Coverage 76.00% 75.84% -0.16%
==========================================
Files 381 381
Lines 34222 34272 +50
==========================================
- Hits 26009 25994 -15
- Misses 6390 6459 +69
+ Partials 1823 1819 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…he EstimateMessageFee method
…n DeployAccount transaction handling
9217d61 to
214dfec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
// session with the benchmark analysis
claude --resume e130bcb2-943a-4697-9228-572919f100c1
Summary
Refactor of the v10
AddTransaction/EstimateFee/Simulate/EstimateMessageFeeflow to:receivedTransactionFeedis enabled.BroadcastedTransaction.ContractClass json.RawMessagewith a typedContractClassstruct that mirrors the spec.EstimateMessageFeefromEstimateFee.What changed
New types
ContractClass,ContractClassEntryPoints,ContractClassEntryPoint(rpc/v10/transaction_types.go) replace the untypedjson.RawMessagefor the broadcastcontract class. Field validation moves from runtime checks (almost no validation here) into struct tags.
MessageFeePayload { L1Handler, PaidFeeOnL1 }(rpc/v10/estimate_fee.go) letsEstimateMessageFeebuild acore.L1HandlerTransactiondirectly without faking aBroadcastedTransaction.Refactored adaptation
AdaptBroadcastedTransactionToCore(ctx, tx, classHash, network)inrpc/v10/adapt_transaction.goreplacesAdaptBroadcastedTransaction. It accepts apre-computed Sierra class hash (declare only) instead of recomputing it, and skips the
BroadcastedTransaction → starknet.Transaction → sn2core.AdaptTransactionroundtrip in favor of dedicated
adaptBroadcastedInvokeToCore/adaptBroadcastedDeclareToCore/adaptBroadcastedDeployAccountToCorehelpers.adaptAndCompileBroadcastedTxToCoreinrpc/v10/transaction.goruns compile + hash + adapt as a single unit, returning(coreTxn, *core.SierraClass, error).addToMempool,prepareTransactions(simulation), and thereceivedTransactionFeedfallback inAddTransactionall share this helper.addToMempoolnow returns the adapted core transaction back toAddTransaction, which forwards it to the feed without re-adapting (and, for declare, withoutrecompiling).
pushToFeederGatewayuses a newContractClassToGatewayPayload(*ContractClass)to produce the gzip+base64 contract class payload from the typed struct, instead of unmarshaling intomap[string]any, mutating, and re-marshaling. Notably this no longer mutates the caller'stx.ContractClass.Spec / API surface
BroadcastedTransaction.PaidFeeOnL1(L1_HANDLER is not broadcastable via RPC).validateResourceBoundstag in favor ofvalidator.WithRequiredStructEnabled()plus plainvalidate:"required".ResourceBoundsMap.{L1Gas,L2Gas,L1DataGas}are now value types (ResourceBounds) instead of*ResourceBounds. Required by validator semantics and removes a layerof nil checks across the call chain.
Performance impact
Benchmarks added under
rpc/v9/bench_addtxn_test.goandrpc/v10/bench_addtxn_test.go(fullHandler.AddTransaction()end-to-end, 8×1s, paired withbenchstat).Stub compiler so the deltas reflect RPC-layer changes only; a separate real-compiler scenario uses
compiler.NewUnsafe()(in-process FFI). Sierra fixture: real8.9k-felt class from
clients/feeder/testdata/sepolia-integration. Both mempool andreceivedTransactionFeedare enabled in every iteration, matching the defaultnode config.
Stub compiler
AddTxn_Mempool_Invoke 250.4 µs 124.5 µs -50.28% -59% -59%
AddTxn_Mempool_DeployAccount 994.8 µs 623.7 µs -37.31% -59% -58%
AddTxn_Mempool_Declare 104.90 ms 42.67 ms -59.32% -90% -84%
AddTxn_Gateway_Invoke 139.7 µs 139.9 µs ~ -7% -11%
AddTxn_Gateway_DeployAccount 399.7 µs 392.2 µs ~ -9% -13%
AddTxn_Gateway_Declare 34.5 ms 72.5 ms +110%* -18% -5%
Real compiler (FFI Sierra→CASM)
AddTxn_Declare_RealCompiler_Mempool 527.7 ms → 255.8 ms -51.53%
AddTxn_Declare_RealCompiler_Gateway 35.6 ms* → 283.1 ms correctness fix, see below
* The v9 gateway-declare numbers are misleading.
pushToFeederGatewayin v9 mutatestx.ContractClassin place (gzipping the Sierra program), and the subsequentreceivedTransactionFeedbranch then tries to unmarshal that mutated payload back into astarknet.SierraClass— which fails (program is now a string, not[]*felt.Felt) and gets logged as a warning. v9 silently drops declare submissions from the received-tx feed when the gateway path is used. v10 doesn't mutate theinput, so the feed branch publishes correctly. The "+110% / +695%" gateway-declare numbers are v10 doing the work v9 was silently skipping.
Where the wins come from on the mempool path
compiler.Compile(Sierra → CASM)sn2core.AdaptSierraClass(Poseidon over program + Keccak over ABI)CalculateSierraClassHash)core.TransactionHashFor nodes acting as both mempool source and feed source — the common production deployment — declare submissions are roughly 2× faster end-to-end with the real
compiler, and use ~65% fewer allocations.
Test plan
go build ./...cleango vet ./rpc/v10/... ./core/... ./rpc/v9/...cleango test ./rpc/v10/... ./core/...passingrpc/{v9,v10}/bench_addtxn_test.go)BenchmarkAddTxn_Declare_RealCompiler_{Mempool,Gateway})receivedTransactionFeedactually emits declare transactions on both pathsFollow-ups (not in this PR)
pushToFeederGatewaycould compute the core txn for the feed at the same time it builds the gateway payload, eliminating the second adapt+compile on the gatewaypath. Would bring v10 gateway-declare from ~283 ms back down to ~35 ms while keeping the feed correct.
adaptCoreResourceBounds/ feeder bounds adapters. Worth promoting torpc/rpccorein a follow-up.ResourceBoundsMap.MarshalJSONdefault-fill should move to a constructor / decode hook rather than mutating a receiver inside a serializer.AdaptBroadcastedTransactionToCore(declare path) andContractClassToGatewayPayload. Two// @todo add testmarkers remain.