flamenco: adds txn generation methods #1692
Conversation
arjain4
changed the title
arjain4
force-pushed
the
arjain/txn_generation
branch
from
0691dc8 to
210ce8d
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
from
210ce8d to
1375ef7
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
from
1375ef7 to
055076a
Compare
|
@ptaffet-jump thank you for the review, this should have been marked as a draft actually. Started cleaning up the code and will look into simplifying these APIs. The first version was just hacked up a few months ago to prove we could send votes to solana. The design comments will be particularly helpful as we start thinking about how we'll use these again |
arjain4
force-pushed
the
arjain/txn_generation
branch
3 times, most recently
from
d1711fe to
a9c25fe
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
from
ef53516 to
d1b764d
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
2 times, most recently
from
b62adff to
54052c1
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
from
54052c1 to
ee2e75e
Compare
arjain4
force-pushed
the
arjain/txn_generation
branch
from
ee2e75e to
b602c52
Compare
| write_ptr += compact_instr_cnt_sz; | ||
|
|
||
| /* Calculate offset of next instruction. */ | ||
| if ( FD_UNLIKELY( txn_meta->instr_cnt > 1 ) ) { |
There was a problem hiding this comment.
I am not sure why this is txn_meta->instr_cnt > 1 instead of txn_meta->instr_cnt > 0. It seems that, even if txn_meta->instr_cnt == 1, we should recalculate write_ptr.
There was a problem hiding this comment.
the write_ptr is already at the right offset for that case
| program_id, | ||
| (ushort)accounts_sz, | ||
| data_sz, acct_off, data_off ); | ||
| return (ulong)(instr_start - out_txn_payload); |
There was a problem hiding this comment.
It seems that you are returning the old txn size instead of the new txn size. Is this deliberate? It seems more natural to return the new txn size.
Also, you never check whether we are writing more than FD_TXN_MTU bytes to out_txn_payload. It may be useful to guarantee that this function never writes overflow out_txn_payload, for security purposes.
There was a problem hiding this comment.
this returns the size of the just added instr
| } | ||
|
|
||
| void | ||
| fd_txn_reset_instrs( uchar * txn_meta_ptr, |
There was a problem hiding this comment.
Maybe call it fd_txn_remove_instrs?
There was a problem hiding this comment.
remove is used for removing a single element across firedancer so i steered clear of that for this
| txn_meta->readonly_unsigned_cnt = accounts->readonly_unsigned_cnt; | ||
| /* Number of signatures is encoded as a compact u16 but | ||
| can always be encoded using 1 byte here. */ | ||
| txn_meta->message_off = (ushort)(num_signatures * FD_TXN_SIGNATURE_SZ + 1); |
There was a problem hiding this comment.
If num_signatures is higher than 20, then this offset is already going beyond FD_TXN_MTU (20*64=1280>1232). It seems useful to guarantee that this function would never write beyond FD_TXN_MTU bytes from out_txn_payload.
There was a problem hiding this comment.
We have it setup such that the caller can specify a larger buffer. That can be changed in a follow up PR if you'd like to take a look at that
Transaction generation for sending votes etc