Deposit sighash collection #381
Conversation
Co-authored-by: Atacan İyidoğan <[email protected]>
Replace arbitrary sleep delays with proper server readiness detection using oneshot channels. This change: - Removes unreliable fixed delays - Adds proper server readiness detection - Enables future shutdown handling capabilities - Makes server startup more deterministic The implementation now uses tokio::sync::oneshot channels to signal when each gRPC server is bound and ready to accept connections.
| let leaf_hash = TapLeafHash::from_script( | ||
| &tx_handler | ||
| .scripts | ||
| .get(txin_index) |
There was a problem hiding this comment.
Do we need bound checks for this? Indexing with [idx] is dangerous if the array is smaller than the index but in this case, is there a possibility that this would happen? If not, we can revert this for simpler code and better performance.
PS: I added this.
| @@ -357,7 +373,7 @@ mod tests { | |||
| let mut tx_handler = create_invalid_mock_tx_handler(actor); | |||
|
|
|||
| let prev_tx: Transaction = Transaction { | |||
| version: Version::TWO, | |||
| version: Version(3), | |||
There was a problem hiding this comment.
Latest rust-bitcoin does not have support for this. That's why we did it like this.
| use futures_core::stream::Stream; | ||
|
|
||
| pub fn create_nofn_sighash_stream( | ||
| _db: Database, | ||
| pub async fn dummy( |
There was a problem hiding this comment.
This function can be deleted: It's here because rust-analyzer won't give hints about code if it is in a macro (in this case try_stream).
| @@ -149,6 +149,7 @@ async fn key_spend() { | |||
| &musig_agg_xonly_pubkey_wrapped, | |||
| ) | |||
| .unwrap(); | |||
| rpc.mine_blocks(1).await.unwrap(); | |||
There was a problem hiding this comment.
This and other unrelated mine_blocks() calls are added because current rust-bitcoin library version creates V2 transactions and Bitcoin Core will deny to use these UTXOs in V3 transactions, before getting them mined. These can be deleted after rust-bitcoin library supports V3 transactions.
| OutPoint { | ||
| txid: wcp_txid, | ||
| vout: watchtower_idx as u32, | ||
| }, |
There was a problem hiding this comment.
I think first input should be vout 0 of watchtower challenge tx, instead of the watchtower challenge page tx
|
|
||
| let mut operator_challenge_nack_txhandler = | ||
| builder::transaction::create_operator_challenge_nack_txhandler( | ||
| wcp_txid, |
There was a problem hiding this comment.
Should be watchtower challenge txid, not page
| // Check for missing indices | ||
| let indices: Vec<i32> = operators.iter().map(|(idx, _, _, _)| *idx).collect(); | ||
| let expected_indices: Vec<i32> = (0..indices.len() as i32).collect(); | ||
|
|
There was a problem hiding this comment.
Is operator_idx consecutive starting from 0 or some random unique identifier for each operator?
core/src/rpc/operator.rs
Outdated
| let winternitz_pubkeys = winternitz_pubkeys | ||
| .into_iter() | ||
| .map(WinternitzPubkey::from_bitvm) | ||
| .collect::<Vec<_>>(); |
|
I suggest holding merge for this PR until we find the reason why grpc_flow test takes too much time. I think test is passing but it is very slow. On Github, it seems to be even more slower and Github marks this test failed because of this. If this is not the case, there could be some race conditions. |
Description
Adds sighash collection for deposit steps.