fix: clearer readiness while node is still syncing#3894
Conversation
- Stratum: wait for sync to finish before accepting miner connections so clients are not confused by early "syncing" / failed job responses. - Foreign API: return Error::Unavailable (HTTP 503) for wallet-facing queries (get_pmmr_indices, get_outputs, get_unspent_outputs) while syncing, instead of misleading NotFound/null. - Preserve descriptive errors when height→PMMR mapping fails. Addresses mimblewimble#3546.
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
The readiness direction makes sense, but the wallet still sees null decode errors because its JSON-RPC client only reads result["Ok"]. The new 503 mapping is not reached by these methods. I think the actual wallet/RPC response should be tested before this is considered fixed.
| end_block_height: Option<u64>, | ||
| ) -> Result<OutputListing, Error> { | ||
| // Wallet scan/info hits this early; return a clear signal while syncing (#3546). | ||
| ensure_not_syncing(&self.sync_state)?; |
There was a problem hiding this comment.
The current grin-wallet client only deserializes result["Ok"] and ignores result["Err"]. This therefore still becomes null and produces the same OutputListing decode error. Could we test this against the actual wallet response handling?
| Error::Argument(msg) => response(StatusCode::BAD_REQUEST, msg), | ||
| Error::RequestError(msg) => response(StatusCode::BAD_REQUEST, msg), | ||
| Error::NotFound => response(StatusCode::NOT_FOUND, "".into()), | ||
| Error::Unavailable(msg) => response(StatusCode::SERVICE_UNAVAILABLE, msg), |
There was a problem hiding this comment.
None of the changed endpoints reaches this REST path. /v2/foreign still returns HTTP 200. Is the 503 mapping meant to be used elsewhere?
| .map_err(|_| Error::NotFound)?; | ||
| .map_err(|e| { | ||
| // Prefer a descriptive error over empty NotFound (wallet misreports null). | ||
| Error::Argument(format!( |
There was a problem hiding this comment.
Internal store or PMMR errors can also reach this path. Could we avoid reporting all chain errors as bad arguments?
| }); | ||
|
|
||
| // We have started | ||
| // We have started (only after sync so is_running matches actual readiness). |
There was a problem hiding this comment.
The listener binds in the spawned thread, so this flag can still become true before bind succeeds. Could startup report success before we mark Stratum as running?
| let handler = Arc::new(Handler::from_stratum(&self)); | ||
| let h = handler.clone(); | ||
|
|
||
| // Wait until the node has finished syncing before accepting miner connections |
There was a problem hiding this comment.
Could we shorten this to describe the current behavior and leave the issue history in the PR?
| start_block_height: u64, | ||
| end_block_height: Option<u64>, | ||
| ) -> Result<OutputListing, Error> { | ||
| // Wallet scan/info hits this early; return a clear signal while syncing (#3546). |
There was a problem hiding this comment.
Could we shorten this to describe the current behavior and leave the issue history in the PR?
| include_proof: Option<bool>, | ||
| _include_merkle_proof: Option<bool>, | ||
| ) -> Result<Vec<OutputPrintable>, Error> { | ||
| ensure_not_syncing(&self.sync_state)?; |
There was a problem hiding this comment.
The current grin-wallet client only reads result["Ok"], so these Err responses still become null and produce decode errors. Could we test all three methods through the actual wallet RPC handling?
2 participants
Summary
Fixes #3546.
API and stratum used to accept connections immediately while the chain was still initialising/syncing. Wallets then saw opaque
NotFound/ null decode errors (e.g. onget_pmmr_indices); miners connected only to get “node is syncing” job failures.Changes
!sync_state.is_syncing(); setis_runningonly thenget_pmmr_indices/get_outputs/get_unspent_outputsreturnError::Unavailable(“node is still syncing…”) while syncingUnavailable→ HTTP 503Test plan
cargo test -p grin_api --lib -- --test-threads=1(includesensure_not_syncing_*)cargo test -p grin_servers --lib mining::stratumserver -- --test-threads=1get_pmmr_indicesshould report syncing / 503, not null NotFound