-
Notifications
You must be signed in to change notification settings - Fork 135
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
SIMD-0248: TPU feedback #248
base: main
Are you sure you want to change the base?
Conversation
proposals/0248-tpu-feedback.md
Outdated
@@ -0,0 +1,178 @@ | |||
--- | |||
simd: '0224' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the SIMD # to 248
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
proposals/0248-tpu-feedback.md
Outdated
} | ||
|
||
The version is a 32 bit unsinged integer, it is set to 1 for first version. It | ||
is designed to allow the protocol to extend for future extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would u8
be enough for version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for foreseeable future u8 should be enough. If we can devise sub version extensions in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once it reaches 256 just assume that any version V < 128 is in fact version 256+V.
timestamp: u32, | ||
transaction_state: TransactionState, | ||
priority_fee_info: PriorityFeeInfo | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt to define two separate feedback types, one for transaction_state
, one for priority_fee_info
?
my reasoning:
transaction_state
is connection oriented, communication is peer-to-peer;TransactionStats
can be sent to Client as soon as become available, or never if Client didn't submit transactions.priority_fee_info
is aggregated at slot level, fits pub/sub system;PriorityFeeInfo
updates can be published at pre-determine frequency.
Separating them into single-function message types provides flexibility in implementation and bandwidth management, perhaps worth the slight message overhead cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allow the same datagram to be able to carry both type of information or priority_fee_info -- so we could reduce datagram count. I think priority_fee_info is always good to have to understand better why it failed. In the case only priority_fee_info is transmitted, the TransactionState's transaction count is set to 0.
If we have a separate message -- we would need another flag field to indicate the message type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your intention to reduce datagram count.
Per our chat offline, it's possible to add 3rd type message to indicate the lowest tx priority Scheduler is accepting. Adding a message_type: u8
to header might not be bad idea; at least different type messages don't have to always go out in same datagram, especially some messages don't have new updates.
priority_fee_info is always good to have to understand better why it failed.
If transaction failed due to prio fee, then the reason should be clear by TransactionStateValue::FeeTooLow
. With priority_fee_info
, user would know "how low".
proposals/0248-tpu-feedback.md
Outdated
transaction is not included in the block. Providing further information on the | ||
transaction state will help the client to make more informed decision on | ||
handling transaction failures and reduce the incentive to blindly resubmitting | ||
transactions and help reduce congestion condition in the network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce the incentive to blindly resubmitting transactions and help reduce congestion condition in the network
imo, by providing feedbacks itself does not necessarily disincentivize spamming; but Server could use TransactionState
to throttle connections with excessively high failure rate, or use PriorityFeeInfo
to drop transactions have priority below min
. Are these reasonable use cases for feedbacks for this SIMD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client can use the information to
- modify the retry behavior of transactions
- proactively not sending transactions which are seriously below the water mark
- percolate the information and set higher fee for transactions to maximize the chance to be included.
Co-authored-by: Tao Zhu <[email protected]>
Co-authored-by: Tao Zhu <[email protected]>
A client connection notifies the server of its interest to receive feedback by | ||
sending a datagram soliciting feedback. This helps server to avoid sending | ||
datagrams to clients which are not interested in receiving feedback or clients | ||
which are not upgraded to have such capabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is best to assume we ask for feedback in the transaction request itself. Sending the feedback request in a separate datagram is adding statful behavior where it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem with that -- the transaction format need to be modified and changes that protocol-- I think we run out of free flags. I think this is a way to show the interest with minimal performance and protocol impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we not have some legacy variants we could reuse?
(transaction, state) pairs | ||
} | ||
|
||
The transactions_count is 8 bit unsgined integer representing the count of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest we use TLV encoding here rather than a fixed format. Typically, TLV encodings provide far better ability for upgrade. Also with TLV we do not need to include total count at all.
|
||
The cost is: | ||
|
||
2500*21*10 bytes /s = 0.8 MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are not counting the headers. IP + UDP+QUIC are already over 30 bytes, so you get some 12 Mbps.
|
||
As not all connections are actively sending transactions, optimization can be | ||
done to reduce the cost to send feedback to clients only having sent | ||
transactions in the last report period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validators should be able to charge fees for this service I think.
The client and server must enable the support of datagrams QUIC frame types | ||
via transport parameter `max_datagram_frame_size`. | ||
|
||
The server sends the feedback datagrams to interested clients periodically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this info should only be provided if a client has submitted a valid transaction in the last 3-4 blocks. Keeping this active for ALL transactions sounds like overkill.
Tagging @apfitzge for comments on this SIMD. |
This document describes the protocol for sending and receiving TPU feedback in solana network.