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

tpu: restructure how txns are passed along #3763

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mmcgee-jump
Copy link
Contributor

No description provided.

@mmcgee-jump mmcgee-jump added this to the Frankendancer milestone Dec 20, 2024
struct fd_txnm {
/* The computed slot that this transaction is referencing, aka. the
slot number of the reference_blockhash. If it could not be
determined, this will be a slot around 150 in the future. */
Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, isn't it just the current slot?

fd_txn_acct_addr_lut_t alut[ ] */
};

typedef struct fd_txnm fd_txnm_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

fd_txn_p_t and fd_txn_e_t have an extra underscore, but they are a bit different. I'd probably still add it.

variable length and not included in the size of this struct.
uchar payload[ ]
fd_txn_t txn_t[ ]
fd_txn_acct_addr_lut_t alut[ ] */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean the expanded address lookup tables? If so, that's not the right type. If you actually mean the field of type fd_txn_acct_addr_lut_t, I'd call that part of the fd_txn_t.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the footprint calculation, I think the type should be fd_acct_addr_t

Comment on lines +130 to +131
/* An 8 byte tag of the first signature in the transaction, for use
by dedup. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this missing on purpose?


/* Can be computed from the txn_t but it's expensive to parse again,
so we just store this redundantly. */
ulong txn_t_sz;
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to make this a ushort to make the padding less ugly. That gives you 4 bytes of trailing padding if that could be useful for anything.

@@ -112,4 +112,95 @@ typedef struct __attribute__((packed)) {
uchar last_entry_hash[32];
} fd_poh_init_msg_t;

/* A fd_txnm_t is a parsed meta transaction, containing not just the
Copy link
Contributor

Choose a reason for hiding this comment

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

There's enough here that you might want to move it to its own file

@@ -201,7 +200,7 @@ fd_topo_initialize( config_t * config ) {
/**/ fd_topob_link( topo, "pack_replay", "pack_replay", 65536UL, USHORT_MAX, 1UL );
/**/ fd_topob_link( topo, "poh_pack", "replay_poh", 128UL, sizeof(fd_became_leader_t) , 1UL );

/**/ fd_topob_link( topo, "replay_voter", "replay_voter", 128UL, FD_TPU_DCACHE_MTU, 1UL );
/**/ fd_topob_link( topo, "replay_voter", "replay_voter", 128UL, FD_TXN_MAX_SZ, 1UL );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean 1232 for this one? FD_TPU_MTU? I don't remember what the format for this link is, but I have a hard time imagining it's just an fd_txn_t.

Copy link
Contributor

@ptaffet-jump ptaffet-jump left a comment

Choose a reason for hiding this comment

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

This is a big improvement. Thanks! A few nits and suggestions, but I like it.

uchar * dst = (uchar *)fd_chunk_to_laddr( ctx->out_mem, ctx->out_chunk );

fd_memcpy( dst, src, sz );
if( FD_UNLIKELY( ctx->in_kind[ in_idx ]==IN_KIND_GOSSIP || ctx->in_kind[ in_idx]==IN_KIND_VOTER ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if( FD_UNLIKELY( ctx->in_kind[ in_idx ]==IN_KIND_GOSSIP || ctx->in_kind[ in_idx]==IN_KIND_VOTER ) ) {
if( FD_UNLIKELY( ctx->in_kind[ in_idx ]==IN_KIND_GOSSIP || ctx->in_kind[ in_idx ]==IN_KIND_VOTER ) ) {

ulong payload_sz = *(ushort*)(dcache_entry + sz - sizeof(ushort));
uchar const * payload = dcache_entry;
fd_txn_t const * txn = (fd_txn_t const *)( dcache_entry + fd_ulong_align_up( payload_sz, 2UL ) );
fd_txnm_t * txnm = (fd_txnm_t *)fd_chunk_to_laddr( ctx->out_mem, ctx->out_chunk );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fd_txnm_t * txnm = (fd_txnm_t *)fd_chunk_to_laddr( ctx->out_mem, ctx->out_chunk );
fd_txnm_t * txnm = (fd_txnm_t *)fd_chunk_to_laddr( ctx->out_mem, ctx->out_chunk );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants