feat(node): gate verify-foreign-tx on available chains via the watch channel - #3912
feat(node): gate verify-foreign-tx on available chains via the watch channel#3912anodar wants to merge 11 commits into
Conversation
…channel The per-request view-client call (chain_is_supported) is gone: request execution now checks a channel-backed supporters map on both the leader and follower paths. foreign_chain_policy::spawn_supporters_by_foreign_chain resolves the indexer's TLS-key channel against the running job's participant set and threshold-filters it (TODO(#3640): switch to the domain threshold); the coordinator spawns it per running job since it needs the current participants. The ReadSupportedForeignChain trait, its real/fake readers, the legacy view-client read, and the reader generic on IndexerAPI/Coordinator/MpcClient/VerifyForeignTxProvider are all removed.
|
@claude review |
Pull request overviewThis PR replaces the on-demand Changes:
Reviewed changesPer-file summary
FindingsNo blocking issues found. A few non-blocking observations: Non-blocking (nits, follow-ups, suggestions):
✅ Approved |
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Left some comments. Not requesting changes because we probably need some discussion around this, and anyway we are not merging this soon.
My main concern is the use of Option in two places.
The first one if for the supported chains. As far I can see, this is a case that cannot happen in practice, once the node is parked on startup until it gets the first set, which it should anyway have as it already needs the contract state to do anything, and these two things are tied together (both as view methods in the contract). I drafted this alternative in #3966 and it seems to work. Happy to hear if there is a downside.
The other place where I think Option might be avoided is in the thresholds in crates/node/src/foreign_chain_policy.rs. The None branch seems to refer to the case where there are no foreign chain domains, but I think we should handle that explicitly, for example by not running the tasks in that case (or some more explicit method).
| // Also checked in `execute_foreign_chain_request`; checked early here | ||
| // because `take_owned` below irreversibly consumes a presignature. An | ||
| // availability flip between the two checks still costs one presignature. | ||
| ensure_chain_is_available( | ||
| &self.supporters_by_foreign_chain.borrow(), | ||
| &foreign_tx_request.request, | ||
| ) | ||
| .inspect_err(|_| metrics::MPC_NUM_VERIFY_FOREIGN_TX_UNAVAILABLE_CHAIN_REJECTIONS.inc())?; |
There was a problem hiding this comment.
I think I never considered this case before, in which something is supported and the not supported. It opens the possibility for race conditions. We probably need to have some robust way of handling this.
There was a problem hiding this comment.
We had similar discussion in #3853 (comment).
Worst case it either times out (if node dropped support for chain that we thought it supported) or we think it's not supported (if node enables support at the "right" time) and fail early, in which case retry will succeed.
We just also check availability (ensure_chain_is_available) here as a way to reduce that race condition window (although not by much), but not trying to eliminate it. Since all nodes are expected to support all the chains, this is similar to node going down/up race condition.
There was a problem hiding this comment.
Speaking of this, it's unfortunate that there's no way to batch multiple view calls into a single atomic request. Feels like it would be a useful feature. I wonder how complex this would be in nearcore to support.
Pull request overviewFollow-up review of the changes since gilcu3's round on 2026-07-24. I verified the four commits that landed after the last review ( gilcu3's feedback → resolution
FindingsNo blocking issues. Non-blocking (nits / follow-ups):
✅ Approved |
a80d3e8 to
93c9ab5
Compare
93c9ab5 to
a80d3e8
Compare
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
Somewhat shallow review, but overall this looks good to me. Thanks!
| tracing::error!(target: "mpc", "error reading foreign-chain supporters from chain: {:?}", e); | ||
| tokio::time::sleep(FOREIGN_CHAIN_SUPPORTERS_REFRESH_INTERVAL).await; |
There was a problem hiding this comment.
I know this predates this PR, but shouldn't we do exponential backoff here? It makes logs cleaner if the node gets stuck on this, and also avoids spamming if we'd get rate limited anywhere.
There was a problem hiding this comment.
I did have exponential backoff initially on that, but changed based on reviewers feedback.
There was a problem hiding this comment.
Hmm I'm not following the reasoning in that thread.
Here, the contract already has all the required API, so emitting an error seems correct - we don't expect this call to fail.
This reasoning seems to assume the contract lacking the API being the only reason for the call to fail. I see several levels of type-erased errors in the view client, so I wouldn't directly trust this to always hold. Are we sure about this? What about network errors? Or other issues in nearcore? How can we be so confident this won't fail? (I know Kevin raised the point, but I assume you had some understanding of this argument when you changed the code)
There was a problem hiding this comment.
How can we be so confident this won't fail?
We can't, and all of the cases you listed will start spamming with error logs.
but I assume you had some understanding of this argument when you changed the code
Only argument against exponential backoff was to save us code complexity. While I don't necessarily agree on it, worst case this will spam 1 log per second so it's not the end of the world. If it keeps spamming long enough (can't read contract state for some reason) we likely have bigger issue (but again not a good reason to avoid backoff).
There was a problem hiding this comment.
worst case this will spam 1 log per second so it's not the end of the world.
I was not aware of this. As an operator, 1 log per second is not the end of the world, but not too far either as it basically makes the log unreadable after a while
| let (sender, receiver) = watch::channel(initial); | ||
| tokio::spawn(async move { | ||
| loop { | ||
| tokio::time::sleep(FOREIGN_CHAIN_SUPPORTERS_REFRESH_INTERVAL).await; |
There was a problem hiding this comment.
Every time we add one of these direct sleeps without properly injecting time, a kitten dies.
Here it would over-complicate the code to try to shoehorn in a proper time abstraction so that's a separate effort. But anyway.
There was a problem hiding this comment.
I had this as config parameter (arguably could have been placed to better place) so that I could set different values in production vs testing code, but changed to hardcoded value as per reviewers suggestion.
Some context in: #3641 (comment), but more was discussed offline on this.
There was a problem hiding this comment.
I don't mind the config parameter, I'm just ranting (off topic) about the lack of dependency injection. Time is I/O and I/O should be separated from business logic. When you don't, any code that touches time interactions becomes incredibly hard to test.
For example, if I have:
fn delayed_add(delay: Duration, a: u64, b: u64) -> u64 {
std::time::sleep(delay);
a.checked_add(b).expect("overflows aren't the point of this example")
}This is a very simple function, but how do we write unit tests for this? Yes, we can ignore the delay, but there's no way to control the delay or test it. In more complex systems this becomes a much more real problem since you can't control the timing between components which leads to huge gaps in what you can and cannot test, and buries interesting code behind race conditions and whatnot.
If I instead simply did:
fn delayed_add(sleep: impl Sleep,delay: Duration, a: u64, b: u64) -> u64 {
sleep.sleep(delay);
a.checked_add(b).expect("overflows aren't the point of this example")
}
trait Sleep {
fn sleep(duration: Duration);
}
struct RealTime {}
impl Sleep for RealTime {
fn sleep(duration: Duration){ std::time::sleep(duration) }
}I could easily inject a fake sleep in tests that records how long the function actually intended to sleep. Additionally I can introduce wait point to fine grained control over which sleep points are passed and when. This makes time testable, and also make unit tests that don't depend on time instant. You should never have to wait for a timeout in tests.
There was a problem hiding this comment.
Fully agree.
In other languages (e.g. Go), I'd stub out foreign_chain_supporters_refresh_interval as a function type variable (maybe even at the package level) and set to different functions depending on production vs testing (pretty close to what you're suggesting). Having time configurable was my way to achieve similar thing in Rust without changing function signature.
I'd rather not refactor this now though as this is existing pattern in the codebase and probably worth something agreeing with others.
| // Also checked in `execute_foreign_chain_request`; checked early here | ||
| // because `take_owned` below irreversibly consumes a presignature. An | ||
| // availability flip between the two checks still costs one presignature. | ||
| ensure_chain_is_available( | ||
| &self.supporters_by_foreign_chain.borrow(), | ||
| &foreign_tx_request.request, | ||
| ) | ||
| .inspect_err(|_| metrics::MPC_NUM_VERIFY_FOREIGN_TX_UNAVAILABLE_CHAIN_REJECTIONS.inc())?; |
There was a problem hiding this comment.
Speaking of this, it's unfortunate that there's no way to batch multiple view calls into a single atomic request. Feels like it would be a useful feature. I wonder how complex this would be in nearcore to support.
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Thank you for all the fixes!
| // Given: four nodes with a mocked Bitcoin RPC (so inspection would | ||
| // succeed) and a ForeignTx domain whose reconstruction threshold is met | ||
| // by all four auto-registrations. |
There was a problem hiding this comment.
nit: we are trying to get all "given,when,then" without further explanatory comments on them, as that should be readable from the code.
There was a problem hiding this comment.
Got rid of the comments in these sections but reworded and added it on top of the test. I think it's useful to have some kind of comments explaining what the test is supposed to be testing.
| fn resolve_supporters_by_foreign_chain( | ||
| supporters_by_tls_key: &ForeignChainSupporters, | ||
| participants_config: &ParticipantsConfig, | ||
| foreign_tx_reconstruction_threshold: u64, |
There was a problem hiding this comment.
nit: I am not sure if this is best, but probably we should use the correct type here ReconstructionThreshold
| .iter() | ||
| .filter_map(|(chain, tls_keys)| { | ||
| let ids = resolve_participant_ids(tls_keys, participants_config); | ||
| (ids.len() as u64 >= foreign_tx_reconstruction_threshold).then_some((*chain, ids)) |
There was a problem hiding this comment.
nit: is this as conversion unavoidable? @netrome did you miss this one or left on purpose?
3 participants
Closes #3910
Followup on #3852
NOTE: this should not be merged until predecessor is not released and deployed (one that registers config to the new endpoint), currently available chains is an empty list.