Skip to content

Sender: Lands UDP votes #5064

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

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

Sender: Lands UDP votes #5064

wants to merge 1 commit into from

Conversation

akhinvasara-jumptrading
Copy link
Contributor

@akhinvasara-jumptrading akhinvasara-jumptrading commented May 9, 2025

This PR cleans up the sender tile such that we can land votes. As of now, they're sent over UDP to the advertised udp vote port. This has been tested against jump's private test cluster.

This is not the final state of sender...there is ongoing work to add quic support and clean up the data structures used. Landing this PR will help further code evolve around this plumbing and for continuous testing with vote=true.


fd_epoch_leaders_t const * lsched = fd_stake_ci_get_lsched_for_slot( ctx->stake_ci, poh_slot );
if( FD_UNLIKELY( !lsched ) ) { return -1; }
if( FD_UNLIKELY( !lsched ) ) {
ctx->metrics.leader_sched_not_found++;
Copy link
Contributor

@mmcgee-jump mmcgee-jump May 12, 2025

Choose a reason for hiding this comment

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

Don't love logging metrics in this function. Just return an error code, and have the parent track the result codes in an enum metric is a little cleaner.

<enum name="GetLeaderResult">
  <Success>
  <ErrNoScheduleForSlot>
  <ErrNoContactInfo>
  <ErrUnroutable>
</enum>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will also change soon, so going to leave as is for now


fd_pubkey_t const * slot_leader = fd_epoch_leaders_get( lsched, poh_slot );
if( FD_UNLIKELY( !slot_leader ) ) { return -1 ; } /* Count this as bad slot too */
if( FD_UNLIKELY( !slot_leader ) ) {
ctx->metrics.leader_not_found++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe per contract, this cannot fail so you can FD_LOG_ERR here and abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will also change with quic PR, and ill incorporate all this feedback in that


fd_shred_dest_t * sdest = fd_stake_ci_get_sdest_for_slot( ctx->stake_ci, poh_slot );
fd_shred_dest_idx_t sdest_idx = fd_shred_dest_pubkey_to_idx( sdest, slot_leader );
if( FD_UNLIKELY( sdest_idx==FD_SHRED_DEST_NO_DEST ) ) {
ctx->metrics.leader_contact_not_found++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also check in this fn that the ip address and port are nonzero, otherwise it's an err (nonroutable) as well.

Copy link
Contributor

@mmcgee-jump mmcgee-jump left a comment

Choose a reason for hiding this comment

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

Thanks! Some good improvements

@akhinvasara-jumptrading akhinvasara-jumptrading force-pushed the akhin/voter branch 3 times, most recently from 3db977d to 4afcafd Compare May 13, 2025 17:14
auto-merge was automatically disabled May 13, 2025 19:47

Pull request was closed

Copy link
Contributor

Choose a reason for hiding this comment

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

This dir should be renamed to discof/send

@@ -25,7 +25,8 @@ class Tile(Enum):
GOSSIP = 19
NETLNK = 20
SOCK = 21,
REPAIR = 22
REPAIR = 22,
SENDER = 23
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
SENDER = 23
SEND = 23,

@@ -210,6 +210,13 @@ metric introduced.
<counter name="PktRetransmissions" summary="Number of QUIC packets that retransmitted." />
</tile>

<tile name="sender">
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
<tile name="sender">
<tile name="send">

Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

Looks good. Just for future reference, I think we should have uniform naming. Either refer to this thing everywhere as send or as sender.

Since the existing name is sender unrelated to this change, I won't block the PR on it.

I haven't reviewed the application logic, so please get an approval from Charlie/Michael, too.

@@ -7,7 +7,7 @@
#define IN_KIND_QUIC (0UL)
#define IN_KIND_BUNDLE (1UL)
#define IN_KIND_GOSSIP (2UL)

#define IN_KIND_SENDER (3UL)
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
#define IN_KIND_SENDER (3UL)
#define IN_KIND_SEND (3UL)

fd_wksp_t * sender_contact_out_mem;
ulong sender_contact_out_chunk0;
ulong sender_contact_out_wmark;
ulong sender_contact_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.

rename to send_

@@ -588,12 +588,12 @@ after_credit( fd_gossip_tile_ctx_t * ctx,

ulong tvu_peer_cnt = 0;
ulong repair_peers_cnt = 0;
ulong voter_peers_cnt = 0;
ulong sender_peers_cnt = 0;
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
ulong sender_peers_cnt = 0;
ulong send_peers_cnt = 0;

err = fd_blockstore_block_hash_query( ctx->blockstore, vote_slot, vote_block_hash );

/* guaranteed to be on frontier from caller check */
const fd_fork_t * fork = fd_forks_query_const( ctx->forks, vote_slot );
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
const fd_fork_t * fork = fd_forks_query_const( ctx->forks, vote_slot );
fd_fork_t const * fork = fd_forks_query_const( ctx->forks, vote_slot );

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.

4 participants