Conversation
- Created TransactionStatusMetaBuilder type for writing transaction metadata to the ledger - Created BlockReward type for block-level reward tracking - Added unix_timestamp (block_time) and rewards fields to SlotState - Updated unit tests to cover new fields and types
- Write TransactionStatusMeta for RPC getBlock/getTransaction - Store block rewards and unix timestamp when rooting slots - Add SPL token parsing and transaction cost model
…lumbing - Thread slot_block_time through clock sysvar updates so replay/rooting can access unix timestamps directly - Adjust getBlock handling to use stored block_time and tighten version checks - Add ExecutedTransaction.total_cost() and use it for CU accounting - Relax u8 index panics in transaction status inner-instruction/token balance builders - Minor cleanups/refactors across replay, cost model, and SPL token parsing
40fae04 to
72d8727
Compare
Specify std.atomic.Value(i64) type on slot_block_time declarations to fix type inference.
07d17c1 to
03340ef
Compare
…acks - Add confirmed commitment path matching Agave's two-phase lookup flow - Fall back to blockstore for block_time, block_height, and rewards when SlotTracker has pruned the slot - Fix Reward.fromLedgerReward to dupe pubkey string instead of parsing
Use unix_timestamp.load(.monotonic) for live slot data accuracy.
e7172e5 to
0568f9e
Compare
e00bb37 to
4e9d457
Compare
7d84370 to
8279fe0
Compare
11ab9b5 to
894fae1
Compare
- Add AccountKeys and ReservedAccountKeys for key segment iteration - Create parse_instruction module with parsers for known programs - Refactor transaction encoding with encodeWithOptions helper - Rename types to use Ui prefix for RPC wire format consistency - Fix transaction version handling in encodeTransactionWithMeta
- Update transaction version to use a labeled block - Rename SPL_ASSOCIATED_TOKEN_ACCOUNT_ID to SPL_ASSOCIATED_TOKEN_ACC_ID - Remove commented-out ParsedInstruction union code - Add doc comment for ParsedInstruction struct - Apply consistent formatting to pubkeyToValue calls
894fae1 to
481b662
Compare
…on metadata - Replace string pubkeys/hashes with typed Pubkey/Hash/Signature throughout - Add FallbackAccountReader to check both writes and account store for mints - Add custom jsonStringify methods for proper Agave-compatible serialization - Remove unnecessary manual memory freeing for typed fields - Fix inner instructions to use UiInstruction union type - Support all transaction encoding formats (binary/base58/base64/json)
2a3553d to
665392e
Compare
- Change UiInstruction.parsed to store a pointer instead of value - Add allocParsed helper to heap-allocate parsed instructions - Reduces union size by avoiding large inline struct
| // ============================================================================ | ||
| // Shared Helpers | ||
| // ============================================================================ |
| fn checkNumAddressLookupTableAccounts( | ||
| accounts: []const u8, | ||
| num: usize, | ||
| ) !void { | ||
| return checkNumAccounts(accounts, num, .addressLookupTable); | ||
| } |
There was a problem hiding this comment.
nit: Can likely inline this bad boy where it's used
| // ============================================================================ | ||
| // BPF Loader v2 Instruction Parser | ||
| // ============================================================================ | ||
|
|
| // ============================================================================ | ||
| // Associated Token Account Instruction Parser | ||
| // ============================================================================ |
| fn checkNumAssociatedTokenAccounts(accounts: []const u8, num: usize) !void { | ||
| return checkNumAccounts(accounts, num, .splAssociatedTokenAccount); | ||
| } |
There was a problem hiding this comment.
nit: can we inline this?
| /// SPL Token instruction tag (first byte) | ||
| const TokenInstructionTag = enum(u8) { | ||
| InitializeMint = 0, | ||
| InitializeAccount = 1, | ||
| InitializeMultisig = 2, | ||
| Transfer = 3, | ||
| Approve = 4, | ||
| Revoke = 5, | ||
| SetAuthority = 6, | ||
| MintTo = 7, | ||
| Burn = 8, | ||
| CloseAccount = 9, | ||
| FreezeAccount = 10, | ||
| ThawAccount = 11, | ||
| TransferChecked = 12, | ||
| ApproveChecked = 13, | ||
| MintToChecked = 14, | ||
| BurnChecked = 15, | ||
| InitializeAccount2 = 16, | ||
| SyncNative = 17, | ||
| InitializeAccount3 = 18, | ||
| InitializeMultisig2 = 19, | ||
| InitializeMint2 = 20, | ||
| GetAccountDataSize = 21, | ||
| InitializeImmutableOwner = 22, | ||
| AmountToUiAmount = 23, | ||
| UiAmountToAmount = 24, | ||
| InitializeMintCloseAuthority = 25, | ||
| // Extensions start at higher values | ||
| TransferFeeExtension = 26, | ||
| ConfidentialTransferExtension = 27, | ||
| DefaultAccountStateExtension = 28, | ||
| Reallocate = 29, | ||
| MemoTransferExtension = 30, | ||
| CreateNativeMint = 31, | ||
| InitializeNonTransferableMint = 32, | ||
| InterestBearingMintExtension = 33, | ||
| CpiGuardExtension = 34, | ||
| InitializePermanentDelegate = 35, | ||
| TransferHookExtension = 36, | ||
| ConfidentialTransferFeeExtension = 37, | ||
| WithdrawExcessLamports = 38, | ||
| MetadataPointerExtension = 39, | ||
| GroupPointerExtension = 40, | ||
| GroupMemberPointerExtension = 41, | ||
| ConfidentialMintBurnExtension = 42, | ||
| ScaledUiAmountExtension = 43, | ||
| PausableExtension = 44, | ||
| _, | ||
| }; |
There was a problem hiding this comment.
nit: Maybe this is annoying, but could link to agave src for where their def is. Same for the other enums in this file.
|
|
||
| // ============================================================================ | ||
| // SPL Token Instruction Parser | ||
| // ============================================================================ |
| ScaledUiAmountExtension = 43, | ||
| PausableExtension = 44, | ||
| _, | ||
| }; |
There was a problem hiding this comment.
Also, might need to check naming convention for enums in Zig. Thing we should prefer camelCase
There was a problem hiding this comment.
applies to the other enums too
There was a problem hiding this comment.
feel free to ignore if this ends up being large change
| fn checkNumTokenAccounts(accounts: []const u8, num: usize) !void { | ||
| return checkNumAccounts(accounts, num, .splToken); | ||
| } |
| for (accounts[last_nonsigner_index + 1 ..]) |signer_idx| { | ||
| try signers.append(try pubkeyToValue( | ||
| allocator, | ||
| account_keys.get(@intCast(signer_idx)).?, |
There was a problem hiding this comment.
the .? usage here may be fine if caller is vetting the bounds (which it is), but still a foot gun i think
204c63f to
90fdd5e
Compare
src/core/transaction.zig
Outdated
| pub fn isMaybeWritable( | ||
| self: Message, | ||
| i: usize, | ||
| reserved_account_keys: ?*const std.AutoHashMapUnmanaged(Pubkey, void), | ||
| ) bool { | ||
| return (self.isWritableIndex(i) and | ||
| !self.isAccountMaybeReserved(i, reserved_account_keys) and | ||
| !self.demoteProgramId(i)); | ||
| } | ||
|
|
||
| pub fn isWritableIndex( | ||
| self: Message, | ||
| i: usize, | ||
| ) bool { | ||
| const num_required_signatures: usize = @intCast(self.signature_count); | ||
| const num_readonly_signed_accounts: usize = @intCast(self.readonly_signed_count); | ||
| if (i < num_required_signatures -| num_readonly_signed_accounts) return true; | ||
|
|
||
| const num_readonly_unsigned_accounts: usize = @intCast(self.readonly_unsigned_count); | ||
| if (i >= num_required_signatures and i < self.account_keys.len -| num_readonly_unsigned_accounts) return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| pub fn isAccountMaybeReserved( | ||
| self: Message, | ||
| i: usize, | ||
| reserved_account_keys: ?*const std.AutoHashMapUnmanaged(Pubkey, void), | ||
| ) bool { | ||
| if (reserved_account_keys) |keys| { | ||
| if (i >= self.account_keys.len) return false; | ||
| return keys.contains(self.account_keys[i]); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| pub fn demoteProgramId( | ||
| self: Message, | ||
| i: usize, | ||
| ) bool { | ||
| return self.isKeyCalledAsProgram(i) and !self.isUpgradeableLoaderPresent(); | ||
| } | ||
|
|
||
| pub fn isKeyCalledAsProgram(self: Message, key_index: usize) bool { | ||
| if (std.math.cast(u8, key_index)) |idx| { | ||
| for (self.instructions) |ixn| { | ||
| if (ixn.program_index == idx) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| pub fn isUpgradeableLoaderPresent(self: Message) bool { | ||
| for (self.account_keys) |account_key| { | ||
| if (account_key.equals(&sig.runtime.program.bpf_loader.v3.ID)) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These functions do the same thing as isWritable. I think it would be easy to simply use isWritable at whatever call site is using these functions.
If this patch easily applies and works can you use it? If there are any issues or complexity that make this insufficient, can you instead add a TODO explaining that it should be consolidated?
Also you added a bunch of pub functions that don't need to be pub.
diff --git a/src/core/transaction.zig b/src/core/transaction.zig
index fec54892..21ce54cf 100644
--- a/src/core/transaction.zig
+++ b/src/core/transaction.zig
@@ -415,71 +415,12 @@ pub const Message = struct {
return index < self.signature_count;
}
- pub fn isMaybeWritable(
- self: Message,
- i: usize,
- reserved_account_keys: ?*const std.AutoHashMapUnmanaged(Pubkey, void),
- ) bool {
- return (self.isWritableIndex(i) and
- !self.isAccountMaybeReserved(i, reserved_account_keys) and
- !self.demoteProgramId(i));
- }
-
- pub fn isWritableIndex(
- self: Message,
- i: usize,
- ) bool {
- const num_required_signatures: usize = @intCast(self.signature_count);
- const num_readonly_signed_accounts: usize = @intCast(self.readonly_signed_count);
- if (i < num_required_signatures -| num_readonly_signed_accounts) return true;
-
- const num_readonly_unsigned_accounts: usize = @intCast(self.readonly_unsigned_count);
- if (i >= num_required_signatures and i < self.account_keys.len -| num_readonly_unsigned_accounts) return true;
-
- return false;
- }
-
- pub fn isAccountMaybeReserved(
- self: Message,
- i: usize,
- reserved_account_keys: ?*const std.AutoHashMapUnmanaged(Pubkey, void),
- ) bool {
- if (reserved_account_keys) |keys| {
- if (i >= self.account_keys.len) return false;
- return keys.contains(self.account_keys[i]);
- }
- return false;
- }
-
- pub fn demoteProgramId(
- self: Message,
- i: usize,
- ) bool {
- return self.isKeyCalledAsProgram(i) and !self.isUpgradeableLoaderPresent();
- }
-
- pub fn isKeyCalledAsProgram(self: Message, key_index: usize) bool {
- if (std.math.cast(u8, key_index)) |idx| {
- for (self.instructions) |ixn| {
- if (ixn.program_index == idx) return true;
- }
- }
- return false;
- }
-
- pub fn isUpgradeableLoaderPresent(self: Message) bool {
- for (self.account_keys) |account_key| {
- if (account_key.equals(&sig.runtime.program.bpf_loader.v3.ID)) return true;
- }
- return false;
- }
-
/// https://github.com/anza-xyz/solana-sdk/blob/5ff67c1a53c10e16689e377f98a92ba3afd6bb7c/message/src/versions/v0/loaded.rs#L118-L150
pub fn isWritable(
self: Message,
index: usize,
lookups: LookupTableAccounts,
- reserved_accounts: *const ReservedAccounts,
+ reserved_accounts: *const sig.utils.collections.PubkeyMap(void),
) bool {
const pubkey = blk: {
if (index < self.account_keys.len) {
diff --git a/src/replay/resolve_lookup.zig b/src/replay/resolve_lookup.zig
index 4cd52338..402a180e 100644
--- a/src/replay/resolve_lookup.zig
+++ b/src/replay/resolve_lookup.zig
@@ -186,7 +186,7 @@ pub fn resolveTransaction(
for (message.account_keys, 0..) |pubkey, i| accounts.appendAssumeCapacity(.{
.pubkey = pubkey,
.is_signer = message.isSigner(i),
- .is_writable = message.isWritable(i, lookups, params.reserved_accounts),
+ .is_writable = message.isWritable(i, lookups, ¶ms.reserved_accounts.map),
});
for (lookups.writable, 0..) |pubkey, i| accounts.appendAssumeCapacity(.{
.pubkey = pubkey,
@@ -194,7 +194,7 @@ pub fn resolveTransaction(
.is_writable = message.isWritable(
message.account_keys.len + i,
lookups,
- params.reserved_accounts,
+ ¶ms.reserved_accounts.map,
),
});
for (lookups.readonly) |pubkey| accounts.appendAssumeCapacity(.{
diff --git a/src/rpc/methods.zig b/src/rpc/methods.zig
index 976b2cdd..9355777a 100644
--- a/src/rpc/methods.zig
+++ b/src/rpc/methods.zig
@@ -2144,7 +2144,11 @@ pub const BlockHookContext = struct {
for (message.account_keys, 0..) |account_key, i| {
accounts[i] = .{
.pubkey = account_key,
- .writable = message.isMaybeWritable(i, &reserved_account_keys.active),
+ .writable = message.isWritable(
+ i,
+ .{ .writable = &.{}, .readonly = &.{} },
+ &reserved_account_keys.active,
+ ),
.signer = message.isSigner(i),
.source = .transaction,
};
diff --git a/src/rpc/parse_instruction/ReservedAccountKeys.zig b/src/rpc/parse_instruction/ReservedAccountKeys.zig
index 542fdee6..e35b1e5f 100644
--- a/src/rpc/parse_instruction/ReservedAccountKeys.zig
+++ b/src/rpc/parse_instruction/ReservedAccountKeys.zig
@@ -6,10 +6,10 @@ const Pubkey = sig.core.Pubkey;
const ReservedAccountKeys = @This();
/// Set of currently active reserved account keys
-active: std.AutoHashMapUnmanaged(Pubkey, void),
+active: sig.utils.collections.PubkeyMap(Pubkey),
/// Set of currently inactive reserved account keys that will be moved to the
/// active set when their feature id is activated
-inactive: std.AutoHashMapUnmanaged(Pubkey, Pubkey),
+inactive: sig.utils.collections.PubkeyMap(Pubkey),
pub fn deinit(self: *ReservedAccountKeys, allocator: std.mem.Allocator) void {
self.active.deinit(allocator);
@@ -18,14 +18,14 @@ pub fn deinit(self: *ReservedAccountKeys, allocator: std.mem.Allocator) void {
// TODO: add a function to update the active/inactive sets based on the current feature set
pub fn newAllActivated(allocator: std.mem.Allocator) !ReservedAccountKeys {
- var active: std.AutoHashMapUnmanaged(Pubkey, void) = .{};
+ var active: sig.utils.collections.PubkeyMap(Pubkey) = .{};
for (RESERVED_ACCOUNTS) |reserved_account| {
try active.put(allocator, reserved_account.key, {});
}
return .{
.active = active,
- .inactive = std.AutoHashMapUnmanaged(Pubkey, Pubkey).empty,
+ .inactive = sig.utils.collections.PubkeyMap(Pubkey).empty,
};
}
src/replay/freeze.zig
Outdated
| collected_transaction_fees: u64, | ||
| collected_priority_fees: u64, | ||
|
|
||
| ledger: *sig.ledger.Ledger, |
There was a problem hiding this comment.
not important, but i would move this up to the shared state section for organization
|
|
||
| /// RPC hook context for block-related methods. | ||
| /// Requires access to the Ledger and SlotTracker for commitment checks. | ||
| pub const BlockHookContext = struct { |
There was a problem hiding this comment.
Can you move this struct exactly as-is into another file? I don't care what it's called but if you're looking for a name you can call it hook_contexts.zig
|
|
||
| const Pubkey = sig.core.Pubkey; | ||
|
|
||
| const ReservedAccountKeys = @This(); |
There was a problem hiding this comment.
can you add a TODO mentioning that this should be consolidated with core.ReservedAccounts
src/runtime/executor.zig
Outdated
| // Clone instruction data so the trace preserves each CPI's data independently. | ||
| // Without this, multiple CPI trace entries can alias the same VM memory region, | ||
| // causing all entries to reflect the last CPI's data. | ||
| // [agave] Uses Cow::Owned(instruction.data) for CPI instructions. | ||
| const owned_data = try tc.allocator.dupe(u8, callee.data); | ||
| errdefer tc.allocator.free(owned_data); |
There was a problem hiding this comment.
I don't see the problem this is supposed to solve. What do you mean by "causing all entries to reflect the last CPI's data"? The data is const so it won't be mutated. If the same pointer was passed into this function, it means you're going to have the same data regardless of whether you clone it. What's the problem with reusing the same pointer when it's already supposed to be the same data?
There was a problem hiding this comment.
After testing this some more, I think the comment should be updated, but removing this change will keep the use after free in the codepath.
Using testnet block 390996596 as an example with transaction 4QUZ1FnLhcD8qzV47sXrAFgrLhrgNhdDT8jfB8kGQxk8n4MkUatWHQNYnZt9MhxR6Co8iDrKzQvLdEqVfupVYDiV:
replayBatch() execution.zig:287
└─ svm_gateway.executeTransaction() svm_gateway.zig:27
└─ loadAndExecuteTransaction() transaction_execution.zig:189
└─ executeTransaction() transaction_execution.zig:427
│ └─ (for each top-level instruction)
│ executeInstruction() executor.zig:21
│ ├─ pushInstruction() executor.zig:62
│ └─ processNextInstruction() executor.zig:131
│ └─ builtin.func() → executor.zig:197
│ execute() execute.zig:31
│ └─ executeBpfProgram() execute.zig:89
│ ├─ serializeParameters() L135
│ ├─ defer serialized.deinit() L142 ◄── [3] FREES buffer on return
│ ├─ sig.vm.init() L158
│ └─ state.vm.run() L169
│ │
│ │ Ho3kdrmwU1poiDRj4pSPtNiceNiyQqzyoXq3TwuJKMrf BPF runs, issues CPI
│ │
│ ├─ invokeSigned() cpi.zig:1022
│ │ ├─ translateInstruction() cpi.zig:734
│ │ │ └─ memory_map.translateSlice() cpi.zig:771
│ │ │ returns []const u8 into serialized buffer
│ │ │ ◄── [1] POINTER OBTAINED
│ │ └─ prepareCpiInstructionInfo() executor.zig:259
│ │ └─ pushInstruction() executor.zig:62
│ │ └─ tc.instruction_trace.append() L106
│ │ ◄── [2] POINTER STORED in trace
│ │
│ └─ (vm.run returns, defers fire)
│ serialized.deinit() fires ◄── [3] BUFFER FREED (0xAA fill)
│ ── trace entries now hold dangling pointers ──
│
└─ returns .instruction_trace = tc.instruction_trace L551
── dangling pointers escape executeTransaction ──
└─ TransactionStatusMetaBuilder.build() Committer.zig:311
└─ convertInstructionTrace() transaction_status.zig:304
└─ convertToInnerInstruction() transaction_status.zig:362
└─ allocator.dupe(u8, ixn_info.instruction_data) L376
◄── [4] READS FROM FREED MEMORY
- Resolve loaded writable/readonly keys from address lookups instead of using empty placeholders - Fix errdefer for owned_loaded_addresses in TransactionStatusMetaBuilder to free individual fields rather than the struct - Add missing errdefer for data allocations in FallbackAccountReader - Fix FallbackAccountReader.get parameter order (allocator, pubkey) - Remove stored allocator from StubAccount, pass explicitly on deinit
90fdd5e to
47b0463
Compare
- Add getRewardsAndNumPartitions to collect vote, stake, and fee rewards per slot - Thread reward_status and block_height through to distributeTransactionFees - Track distributed stake rewards during partition distribution - Store vote rewards in EpochRewardStatus for epoch boundary recording
- Deinit inner items of inner_instructions, pre/post_token_balances before freeing slices - Change pre/post token balance cleanup from defer to errdefer in Committer since ownership transfers to TransactionStatusMeta
- Add exactFloat helpers to format f64 with decimal point (e.g. "3.0" instead of "3e0") - Use number_string instead of float for uiAmount in parsed token balances - Update test expectation to reflect correct "1.0" serialization
- Deinit inner items of inner_instructions, pre/post_token_balances before freeing slices - Change pre/post token balance cleanup from defer to errdefer in Committer since ownership transfers to TransactionStatusMeta
cdf0a23 to
1890b73
Compare
… program - Add errdefer for inner instruction to prevent leak on append failure - Remove unnecessary dupe of log_messages, use meta field directly - Use allocator.dupe with inline array literals in system program helpers
…256r1 cost model
- Change resolveTokenBalances return type to error{OutOfMemory}- Use appendAssumeCapacity since list is pre-allocated to exact size
- Add catch null at Committer call sites to preserve existing behavior
- Enable secp256r1 precompile signature cost calculation in cost_model
- Minor style cleanups in spl_token helpers
- Add ED25519_VERIFY_STRICT_COST constant alongside existing ED25519_VERIFY_COST
- Add remove_simple_vote_from_cost_model to features.zon
- Gate simple vote static cost on remove_simple_vote_from_cost_model feature
- Pass feature_set and slot to cost model functions for feature-gated behavior
- Use strict ed25519 verify cost (2400 CU) only when feature is active, non-strict (2280 CU) otherwise
- Delete duplicate ReservedAccountKeys in rpc/parse_instruction - Add initAllActivated constructor to core ReservedAccounts - Add errdefer to initForSlot for proper cleanup on update failure - Change ACCOUNTS from slice to array type for use with .len - Update LoadedMessage and transaction.Message to use PubkeyMap(void) - Remove stale commented-out code in ParsedInstruction
- Remove LoadedMessage wrapper and its writable cache in favor of calling Message.isWritable with optional lookups - Remove duplicate isMaybeWritable/isWritableIndex/demoteProgramId helpers from Message - Simplify EncodedTransaction.binary to use anonymous struct (tuple) - Remove unused buildPartiallyDecoded function from parse_instruction - Reorder FinalizeStateParams fields for grouping clarity - Improve CPI instruction data clone comment to explain use-after-free risk
Intent
getBlockRPC methodImplementation
Notable Validator Changes
sig.core.transaction.Messagefor RPC encodingcost_unitsfield tosig.ledger.transaction_status.TransactionStatusMetaProcessedTransactionTransactionStatusMetaBuilderhelper type for writing transactions toLedgerspl_token.zigandcost_model.zigfor populatingcostUnitsandcomputeUnitsConsumedfieldsCommitter.commitTransactionsto write transactions toLedgerdistributeTransactionFeesto write rewards toLedgerupdateClockandupdateSysvarsForNewSlotto returnClockforblock_timewriting toLedgerprocessNextInstructionto only invoke non-precompile programsInstructionTraceentries with a depth greater than 1 to now be cleaned up byTransactionContextRPC Changes
parse_instructionmodule for handling transaction instruction parsingAccountKeys,LoadedMessage, andReservedAccountKeystypes inparse_instructionto better match testnet functionalitygetBlockand any future Ledger-related methodsRamifications
AccountKeys,LoadedMessage, andReservedAccountKeysshould be removed in favor of custom handling in encoding functions and utilizingsig.core.ReservedAccountsinstead, but should be done carefully to ensure no loss of functionality.parse_instructionmodule can be split up into modules instead all functions living inparse_instruction/lib.zig, while additionally creating concrete type definitions for each inline JSON object being constructed for each typeGetBlock.Responsetypes andgetBlockhelper functions should be moved into their ownparse_transactionorledger_codecmodule to separate RPC method logic from encoding/serializationTests