-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix(sequencer-relayer): poll Celestia App for transaction status #1940
base: main
Are you sure you want to change the base?
Conversation
|
||
sequencer_relayer | ||
.timeout_ms( | ||
11_000, |
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 is an abhorrently long time for a test to take, but we need to test that the relayer correctly exits if the allotted time passes without the status changing from "UNKNOWN". We cannot advance time manually due to the multithreaded context, so I'm not sure of a way around this :/
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.
We could perhaps change the actual timeout based on the test
feature:
#[cfg(test)]
const MAX_WAIT_FOR_UNKNOWN: Duration = Duration::from_secs(2);
#[cfg(not(test))]
const MAX_WAIT_FOR_UNKNOWN: Duration = Duration::from_secs(10);
Alternatively, 10 seconds might be too long anyway given that Celestia block time is now 6 seconds?
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.
In 4f304bb I lowered the time to 6s and the minimum logging time to 3. I'm not sure how I feel about building the testing functionality into the source code, but I'll defer to you here on what you think is best, since I think I lack the context of what is best practice. I feel like generally, though, it's best to keep testing logic (especially for blackbox tests) separate
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.
Mostly looking good. Just a few comments.
crates/astria-sequencer-relayer/src/relayer/celestia_client/error.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/celestia_client/error.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/celestia_client/error.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/celestia_client/error.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/celestia_client/error.rs
Outdated
Show resolved
Hide resolved
// The min seconds to sleep after receiving a TxStatus response and sending the next | ||
// request. | ||
const POLL_INTERVAL_SECS: u64 = 1; | ||
// The minimum duration between logs. | ||
const LOG_INTERVAL: Duration = Duration::from_secs(5); | ||
// The maximum amount of time to wait for a transaction to be committed if its status is | ||
// `UNKNOWN`. | ||
const MAX_WAIT_FOR_UNKNOWN: Duration = Duration::from_secs(10); |
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.
Just wondering about the rationale for changing the retry backoff and removing the START_LOGGING_DELAY
? Seems like that behaviour is still desirable?
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 suppose I thought because this functionality was moved to the debug level, it could be helpful to have the full picture of each response the relayer has received. I don't have a strong opinion on this, though, so I'd be happy to add that back if you think it's better
crates/astria-sequencer-relayer/tests/blackbox/helpers/mock_celestia_app_server.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/tests/blackbox/helpers/mock_celestia_app_server.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/tests/blackbox/helpers/mock_celestia_app_server.rs
Outdated
Show resolved
Hide resolved
|
||
sequencer_relayer | ||
.timeout_ms( | ||
11_000, |
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.
We could perhaps change the actual timeout based on the test
feature:
#[cfg(test)]
const MAX_WAIT_FOR_UNKNOWN: Duration = Duration::from_secs(2);
#[cfg(not(test))]
const MAX_WAIT_FOR_UNKNOWN: Duration = Duration::from_secs(10);
Alternatively, 10 seconds might be too long anyway given that Celestia block time is now 6 seconds?
Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>
Summary
Changes polling method for confirming Celestia commitment from
GetTx
gRPC toTxStatus
.Background
Previously,
GetTx
was polled indefinitely. On the client end of this RPC, we'd have no indication of what's happening on Celestia, so we had no way to know if a transaction was, for instance, evicted from the mempool, which would cause a halt of data submission. TheTxStatus
endpoint solves this problem by providing a more detailed status of the transaction.Changes
TxStatus
instead ofGetTx
. The client will behave in the following ways:PENDING
: continue pollingUNKNOWN
: continue polling untilMAX_WAIT_TIME_UNKNOWN
is reached (currently 6s)COMMITTED
: return block heightEVICTED
: return an errorTesting
Changelogs
Changelog updated.
Metrics
Related Issues
closes #1936