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

Extend transaction subscription to include block_time #228

Open
vovkman opened this issue Oct 30, 2023 · 11 comments
Open

Extend transaction subscription to include block_time #228

vovkman opened this issue Oct 30, 2023 · 11 comments
Assignees

Comments

@vovkman
Copy link
Contributor

vovkman commented Oct 30, 2023

Currently the transaction notification doesn't include block_time, however this is included in the standard RPC interface here. A few options

  1. Update SubscribeUpdateTransaction to include optional uint64 block_time field that gets set based on existence of block_meta in the SlotsMessages
  2. Same as 1 but update SubscribeRequestFilterTransactions with optional bool block_meta_required field that defaults to false

if block_meta_required is true

  • Prevent txn broadcast until block_meta is present

else

  • broadcast whether block_meta is present or not

Could be argued that this should be done client side, but I prefer reducing as many messages as possible at the source

@vovkman
Copy link
Contributor Author

vovkman commented Nov 6, 2023

cc @fanatid

@kespinola
Copy link
Collaborator

Giving this a look today. I'll report back on what I find.

@linuskendall
Copy link
Contributor

linuskendall commented Nov 8, 2023 via email

@kespinola
Copy link
Collaborator

kespinola commented Nov 8, 2023

@vovkman the block time is made available in the slot update event but not the transaction. It does have the slot in which case getBlockTime can be passed for fined the block_time (which you may have already known but adding to the record)

https://docs.solana.com/api/http#getblocktime

@linuskendall
Copy link
Contributor

linuskendall commented Nov 8, 2023

No need for getBlockTime, the block subscription data already provides this via gRPC :

This subscription would give access to both transactions and block_time since the block meta message would include the block time for the specific transactions:

{"slots": {}, "accounts": {}, "transactions":  { "serum": { "vote": false, "failed": false, "account_include": [ "9xQeWvG816bUx9EPjHmaT23yvVM2ZWbrrpZb9PusVFin" ]}}, "blocks": {}, "blocks_meta": { "blockmetadata": {} }, "accounts_data_slice": []}

IF we want to add block_time to the transaction it should definitely be a flag and also something that clearly indicates that it will slow down transaction delivery (e.g. wait_for_block_meta or sth).

@vovkman
Copy link
Contributor Author

vovkman commented Nov 8, 2023

Yeah this is meant to simplify things for clients. I think the wait_for_block_meta flag is a good idea, and it can be clearly called out in docs what it does

@kespinola
Copy link
Collaborator

@vovkman would a client cache that buffered and then flushed the transactions once the block be confirmed suffice for your use case? This would allow you to have access to the block metadata at the same time as processing the transaction but does add delay to the processing.

@vovkman
Copy link
Contributor Author

vovkman commented Nov 8, 2023

I would prefer for the changes to be in the plugin itself. Right now the plugin works for power users, but I don't see a downside to adding more logic to make it simple for regular users too

@kespinola
Copy link
Collaborator

@vovkman @linuskendall here is a pros and cons list as I see it:

  • Both allow options have the benefit of preventing the processing transactions until the block is confirmed. Currently transactions are passed through the plugin but may not reach confirmation.

Wait for block flag

Pro

  • No extra setup on the implementor who wants to receive transaction updates only after the block has been confirmed

Con

  • Buffering transactions on the server would need some persistence to aid against server crash. If the clients are waiting but the server holding their subscription crashes then all transactions for the block will be lost unless intermediate storage added.
  • Minor con but additional configuration that the implementor needs to be aware of even if they may not want to leverage the feature.
  • Additional conditional logic on the server that may introduce performance issues or bugs from additional complexity. (This may also not be the case but noting as their is a chance)

Client side transaction cache+flush

Pros

  • Compose extra functionality vs configuring it.
  • Allows callers to keep a cache of previous blocks that that can call upon for other use cases.

Con

  • Setting up the cache+flush requires extra code on the client.
  • The cache needs to be reimplemented in each supported language (eg rust, go, js)
  • Its the client's responsibility to protect against crashes. If their server crashes the loose all the transactions and would need to have backup code to pull from RPC.

@vovkman
Copy link
Contributor Author

vovkman commented Dec 4, 2023

I am personally more in favor of Wait for block flag, the pros of the second option can still be leveraged if clients want it as a separate feature. The cons about persistence/crashing is present for all subscriptions currently, and perf can be tested, I don't suspect any issue since the plugin buffers the last 10 SlotMessages already anyways

@linuskendall
Copy link
Contributor

linuskendall commented Dec 7, 2023

I am personally more in favor of Wait for block flag, the pros of the second option can still be leveraged if clients want it as a separate feature. The cons about persistence/crashing is present for all subscriptions currently, and perf can be tested, I don't suspect any issue since the plugin buffers the last 10 SlotMessages already anyways

We will need to buffer all the transactions for the slot and flush them so it definitely means a lot more server side buffering for the client in this mode (however: confirmed and finalzied anyway has the same semantics so I don't think it will be a huge difference).

I think having a flag is a little bit inelegant, but maybe it's the best way to do it? another way would be to specify a unique commitment level (processed_complete?). In either case, the presence of this mode (flag or commitment level) would result in:

  • for transactions, buffer the transactions until the blockmeta is available and flush them with the blockmeta included
  • for accounts, buffer the latest account writes (by write version) and flush one account write per block with the blockmeta incldued

The benefit of making this a commitment level type thing is that commitment levels already mean "how fast/slow will i receive these updates" (in addition to meaning something about the forking states/validity).

@fanatid fanatid self-assigned this Nov 11, 2024
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

No branches or pull requests

4 participants