Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/src/foreign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::handlers::blocks_api::{BlockHandler, HeaderHandler};
use crate::handlers::chain_api::{ChainHandler, KernelHandler, OutputHandler};
use crate::handlers::pool_api::PoolHandler;
use crate::handlers::transactions_api::TxHashSetHandler;
use crate::handlers::utils::ensure_not_syncing;
use crate::handlers::version_api::VersionHandler;
use crate::pool::{self, BlockChain, PoolAdapter, PoolEntry};
use crate::types::{
Expand Down Expand Up @@ -236,6 +237,7 @@ where
include_proof: Option<bool>,
_include_merkle_proof: Option<bool>,
) -> Result<Vec<OutputPrintable>, Error> {
ensure_not_syncing(&self.sync_state)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

let output_handler = OutputHandler {
chain: self.chain.clone(),
};
Expand Down Expand Up @@ -267,6 +269,7 @@ where
max: u64,
include_proof: Option<bool>,
) -> Result<OutputListing, Error> {
ensure_not_syncing(&self.sync_state)?;
let output_handler = OutputHandler {
chain: self.chain.clone(),
};
Expand All @@ -288,6 +291,8 @@ where
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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we shorten this to describe the current behavior and leave the issue history in the PR?

ensure_not_syncing(&self.sync_state)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

let txhashset_handler = TxHashSetHandler {
chain: self.chain.clone(),
};
Expand Down
8 changes: 7 additions & 1 deletion api/src/handlers/transactions_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,13 @@ impl TxHashSetHandler {
let chain = w(&self.chain)?;
let range = chain
.block_height_range_to_pmmr_indices(start_block_height, end_block_height)
.map_err(|_| Error::NotFound)?;
.map_err(|e| {
// Prefer a descriptive error over empty NotFound (wallet misreports null).
Error::Argument(format!(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal store or PMMR errors can also reach this path. Could we avoid reporting all chain errors as bad arguments?

"cannot map block heights {}..{:?} to PMMR indices: {}",
start_block_height, end_block_height, e
))
})?;
let out = OutputListing {
last_retrieved_index: range.0,
highest_index: range.1,
Expand Down
41 changes: 41 additions & 0 deletions api/src/handlers/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use crate::chain;
use crate::chain::types::CommitPos;
use crate::chain::SyncState;
use crate::core::core::OutputIdentifier;
use crate::rest::*;
use crate::types::*;
Expand All @@ -29,6 +30,18 @@ pub fn w<T>(weak: &Weak<T>) -> Result<Arc<T>, Error> {
.ok_or_else(|| Error::Internal("failed to upgrade weak reference".to_owned()))
}

/// Fail wallet-oriented queries while the node is still syncing so clients get a
/// clear "try again later" signal instead of a misleading NotFound (#3546).
pub fn ensure_not_syncing(sync_state: &Weak<SyncState>) -> Result<(), Error> {
let sync = w(sync_state)?;
if sync.is_syncing() {
return Err(Error::Unavailable(
"node is still syncing; wait until sync completes and retry".into(),
));
}
Ok(())
}

/// Internal function to retrieves an output by a given commitment
fn get_unspent(
chain: &Arc<chain::Chain>,
Expand Down Expand Up @@ -88,3 +101,31 @@ pub fn get_output_v2(

Ok(Some((output_printable, out)))
}

#[cfg(test)]
mod tests {
use super::*;
use crate::chain::types::SyncStatus;
use crate::chain::SyncState;
use std::sync::Arc;

#[test]
fn ensure_not_syncing_ok_when_no_sync() {
let sync = Arc::new(SyncState::new());
sync.update(SyncStatus::NoSync);
let weak = Arc::downgrade(&sync);
assert!(ensure_not_syncing(&weak).is_ok());
}

#[test]
fn ensure_not_syncing_unavailable_while_syncing() {
let sync = Arc::new(SyncState::new());
// Default SyncState is Initial → is_syncing() true
let weak = Arc::downgrade(&sync);
let err = ensure_not_syncing(&weak).unwrap_err();
match err {
Error::Unavailable(msg) => assert!(msg.contains("syncing")),
other => panic!("expected Unavailable, got {:?}", other),
}
}
}
4 changes: 4 additions & 0 deletions api/src/rest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ pub enum Error {
Argument(String),
#[error("Not found.")]
NotFound,
/// Node is not ready yet (e.g. still syncing). Prefer this over [`NotFound`]
/// so clients can distinguish "missing data" from "try again later" (#3546).
#[error("Service unavailable: {0}")]
Unavailable(String),
#[error("Request error: {0}")]
RequestError(String),
#[error("ResponseError error: {0}")]
Expand Down
1 change: 1 addition & 0 deletions api/src/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ where
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),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the changed endpoints reaches this REST path. /v2/foreign still returns HTTP 200. Is the 503 mapping meant to be used elsewhere?

Error::Internal(msg) => response(StatusCode::INTERNAL_SERVER_ERROR, msg),
Error::ResponseError(msg) => response(StatusCode::INTERNAL_SERVER_ERROR, msg),
Error::Router { .. } => response(StatusCode::INTERNAL_SERVER_ERROR, "".into()),
Expand Down
19 changes: 13 additions & 6 deletions servers/src/mining/stratumserver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -878,11 +878,23 @@ impl StratumServer {
let handler = Arc::new(Handler::from_stratum(&self));
let h = handler.clone();

// Wait until the node has finished syncing before accepting miner connections

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we shorten this to describe the current behavior and leave the issue history in the PR?

// (#3546). Previously we listened early and returned confusing "syncing" /
// failed job responses while the chain was still catching up.
info!(
"(Server ID: {}) Stratum waiting for node sync before listening on {}",
self.id,
self.config.stratum_server_addr.clone().unwrap()
);
while self.sync_state.is_syncing() {
thread::sleep(Duration::from_millis(50));
}

let _listener_th = thread::spawn(move || {
accept_connections(listen_addr, h);
});

// We have started
// We have started (only after sync so is_running matches actual readiness).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mut stratum_stats = self.stratum_stats.write();
stratum_stats.is_running = true;
Expand All @@ -895,11 +907,6 @@ impl StratumServer {
self.config.stratum_server_addr.clone().unwrap()
);

// Initial Loop. Waiting node complete syncing
while self.sync_state.is_syncing() {
thread::sleep(Duration::from_millis(50));
}

handler.run(&self.config, &self.tx_pool);
} // fn run_loop()
} // StratumServer
Expand Down
Loading