tile, gossip: rewire dedup out to verify #3692
Conversation
ravyu-jump
force-pushed
the
ravyu/gossip-fix-vote-verify
branch
from
af5fe49 to
6c972a9
Compare
src/app/fdctl/run/tiles/fd_dedup.c
Outdated
| @@ -16,8 +16,17 @@ | |||
| checks the transaction signature field for duplicates and filters | |||
| them out. */ | |||
|
|
|||
| #define GOSSIP_IN_IDX (0UL) /* Frankendancer and Firedancer */ | |||
| #define VOTER_IN_IDX (1UL) /* Firedancer only */ | |||
| #ifdef FD_HAS_NO_AGAVE | |||
There was a problem hiding this comment.
Not the correct way to do this ... why can't you keep the current mechanism?
There was a problem hiding this comment.
why can't you keep the current mechanism?
do you mean:
#define GOSSIP_IN_IDX (0UL) /* Frankendancer only */
#define VOTER_IN_IDX (0UL) /* Firedancer only */
Without topology client awareness we need to make the check for gossip link optional. The current check (in unprivileged_init) makes it mandatory for both topologies. I didn't want to break that assumption so I went this way. But if it's not actually a strict requirement I can change that.
I'm also curious: what's the correct way to weave in client-aware blocks of code?
There was a problem hiding this comment.
The FD_HAS_NO_AGAVE flag is a bit problematic, since it creates mismatching build artifacts. These tiles should be generic in the sense that they could run in either full FD or Frankendancer without needing a recompile.
You can achieve that pretty easily here in dedup, by looking at the names of the links it's passed, and handling them accordingly. See for example poh tile fd_poh.c:2025 or pack fd_pack.c:806 for a slightly cleaner way to initialize / manage the in links. Sorry, haven't got around to removing the boilerplate yet.
ravyu-jump
force-pushed
the
ravyu/gossip-fix-vote-verify
branch
2 times, most recently
from
8c23020 to
e79754b
Compare
src/app/fdctl/run/tiles/fd_gossip.c
Outdated
| ulong vote_txn_sz = gossip_vote->txn.raw_sz; | ||
| memcpy( vote_txn_msg, gossip_vote->txn.raw, vote_txn_sz ); | ||
|
|
||
| /* DEFENSE-IN-DEPTH: While the underlying txn message is technically verifed |
There was a problem hiding this comment.
This comment doesn't seem to make sense. You've only verified one signature with the CRDS message? And it might be for bytes that aren't the actual transaction bytes. Also what does the TODO mean? What impact would it have on "verify traffic" ?
There was a problem hiding this comment.
You've only verified one signature with the CRDS message
Yeah you're right.
Also what does the TODO mean? What impact would it have on "verify traffic" ?
My understanding is that the verify tile(s) is performance sensitive and that gossip votes (which are a significant portion of gossip traffic) being funneled there might have some impact? This was based off a hunch that I wanted to investigate at some point. Leaving it as a TODO here is slightly misleading in that there is no code changes to be done here. I'll remove it.
There was a problem hiding this comment.
There is no performance impact of sending gossiped votes to sigverify, it's not a material volume, and you need to verify them... verifying the CRDS is not the same as verifying the transaction.
src/app/fdctl/run/tiles/fd_dedup.c
Outdated
| @@ -41,6 +38,7 @@ typedef struct { | |||
| /* The first unparsed_in_cnt in links do not have the parsed fd_txn_t | |||
| in the payload trailer. */ | |||
| ulong unparsed_in_cnt; | |||
| ulong gossip_in_idx; | |||
There was a problem hiding this comment.
Can you just do an ulong in_kind[ 64UL ] here the same as poh instead? I think it's much cleaner.
ravyu-jump
force-pushed
the
ravyu/gossip-fix-vote-verify
branch
2 times, most recently
from
caaa77c to
d5a7b2e
Compare
src/app/fdctl/run/tiles/fd_dedup.c
Outdated
| tile->in_cnt, i, topo->links[ tile->in_link_id[ i ] ].name )); | ||
| } | ||
| } | ||
| if( FD_UNLIKELY( tile->in_cnt == 0UL ) ) { |
There was a problem hiding this comment.
I wouldn't even bother to check this, topology constructor can do what they like.
ravyu-jump
force-pushed
the
ravyu/gossip-fix-vote-verify
branch
from
d5a7b2e to
801b3af
Compare
No description provided.