feat(node): chain-compatible presignature selection for verifying foreign transactions - #4054
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.
6475bb8 to
bec2c49
Compare
haiyuechen-nearone
left a comment
haiyuechen-nearone
left a comment
There was a problem hiding this comment.
Feel free to dismiss the suggestions if I misunderstood
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
Thanks for the well-sliced PR. I find it difficult to navigate and understand the asset queue, so I'm happy I don't have to consider anything else here. I still don't understand everything going on here but the intention is clear, and most of the code looks reasonable.
One blocker though: The .expect() on the rocksdb commit error is giving me a bit of a pause - couldn't we handle this error instead.
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
Thanks for the updates, and creating the follow-up 🙏
kevindeforth
left a comment
kevindeforth
left a comment
There was a problem hiding this comment.
We had a little attestation expiration emergency which interrupted me mid-review. Will pick this up tomorrow.
…pc into anodar/3569-6-chain-aware-presigs
There was a problem hiding this comment.
Unfortunately, we are running into limitations of our existing asset queue implementation.
The currently proposed solution may lead to delayed signature responses in case the network does not have a uniform foreign chain tx support (#4054 (comment)).
A proper solution will probably require a refactor of the asset queue, so it's a trade-off:
- We ship it and gain more robust foregin-chain tx requests, at the risk of delayed signature responses and possibly at the cost of future refactors.
- We take time to refactor the queue now and solve other existing problems with it (such as offline participants), at the cost of more stringent requirements wrt to foreign chain configs in the MPC network.
I think I have a preference for 2 here, as we now have much better tooling for the foreign chain configs and I think the asset queue has been a thorn in our eye for a long time.
But I don't have full visibility on the demands of the community / business.
In any case, I think his is @netrome's call, as it's essentially a choice between shipping a feature now at the cost of increased tech-debt, or addressing tech-debt at the cost of delaying the feature.
Remark: the required refactor might be smaller than it might seem here. I haven't thought too much about possible designs yet, but I can if that is desired.
|
As per our offline discussion, adding notifies to mitigate front-running problem. |
| }; | ||
|
|
||
| if ingested { | ||
| self.cold_queue_new_elements.notify_waiters(); |
There was a problem hiding this comment.
Note that we are waking ourselves here. Not ideal, but it's okay.
5 participants
Extends queue with
take_first_matchingandingestUnlike
take,take_first_matchingdoes full scan (accepted degradation risk when nodes drop support for given chain).Closes: #3959