fix(sequencer-relayer): poll Celestia App for transaction status #1940
Conversation
|
|
||
| sequencer_relayer | ||
| .timeout_ms( | ||
| 11_000, |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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>
crates/astria-sequencer-relayer/src/relayer/celestia_client/mod.rs
Outdated
Show resolved
Hide resolved
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or this PR will be |
Summary
Changes polling method for confirming Celestia commitment from
GetTxgRPC toTxStatus.Background
Previously,
GetTxwas 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. TheTxStatusendpoint solves this problem by providing a more detailed status of the transaction.Changes
TxStatusinstead ofGetTx. The client will behave in the following ways:PENDING: continue pollingUNKNOWN: continue polling untilMAX_WAIT_TIME_UNKNOWNis reached (currently 7s)COMMITTED: return block heightEVICTED: return an errorTesting
Changelogs
Changelog updated.
Metrics
Related Issues
closes #1936