Validate reject-withdrawal-request #1336
Conversation
djordon
added
withdrawal
xoloki
force-pushed
the
478-validate-reject-withdrawal-request
branch
2 times, most recently
from
353028d to
8fe2632
Compare
xoloki
changed the base branch from
main
to
1308-implement-query-returning-withdrawalrequestreport
…wo checks call iter not into_iter for lint check check that the withdrawal-request is final on the btc chain use corrent block hash in storage requests and errors use proper chain tip errors create withdrawal-reject integration tests based on the withdrawal-accept tests log error since we don't have good error returns yet clean up error handling disable most reject tests but leave in the ones that are analaguous to the accept ones don't clone if you can copy add some temporary logging use block heights to determine if 6 blocks have passed since the withdrawal request failure to get withdrawal signers or request report should map to RequestMissing remove unused tests; use bitmap mismatch test move QualifiedRequestId into common proto; RejectWithdrawalV1 now has a QualifiedRequestId in both code and proto; deal with fallout fix tests to use QualifiedRequestId for RejectWithdrawalV1 map the DB error to the expected type remove stacks/common.proto since the linter complained it wasn't being used check reject threshold use TestSweepSetup2 to try to fix problems with the signer set length address review comments, though they seem to have made things worse use sweep block height not fake data in TestSweepSetup2 hard wire the correct signing threshold into ReqContext add TestSweep2 reject_withdrawal_request and call it from happy path test
xoloki
force-pushed
the
478-validate-reject-withdrawal-request
branch
from
6c3a70b to
e20193c
Compare
|
Ok @djordon I have it all working now, with minimal changes to |
signer/src/stacks/contracts.rs
Outdated
| if rejected_count < reject_threshold { | ||
| return Err( | ||
| WithdrawalRejectErrorMsg::WithdrawalRequestNotRejected.into_error(req_ctx, self) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This part is tricky since there are two reasons for why we would reject a withdrawal.
- The withdrawal request has enough votes against.
- The withdrawal request has expired. This happens after
WITHDRAWAL_BLOCKS_EXPIRYblocks (the one from my PR here) from the height of thebitcoin_block_heightin theWithdrawalRequestReport(it'sblock_heighton main but we're renaming it).
So we need to check for either condition here.
There was a problem hiding this comment.
I don't think that this one is done.
… get a SQL error in the conversion
… use make_withdrawal_reject2 in reject_withdrawal_validation_bitmap_mismatch so it will have the chain tip in req_ctx; use req_ctx to get chain tip in validate
There was a problem hiding this comment.
We need to make add another check here, specifically, the reject-withdrawal-request transaction is valid if the request is expired or collectively rejected. We need to add the expired check, which is check to see if 24 or more blocks have arrived after the bitcoin_block_height of the withdrawal request. That field is also in the withdrawal request report.
And we need a few more tests here. In general, we should aim for a test of each of the different validation conditions that we check for, so basically one test per WithdrawalRejectErrorMsg variant. That might seem like a lot, but I believe that there are several error variants that we can remove after some of the recent edits; they're currently unused. And we don't need to add a test for the MissingStacksBlock at all, I think we can remove that variant in another PR.
| let tip_block_hash = db.get_bitcoin_canonical_chain_tip().await.unwrap().unwrap(); | ||
| let tip_bitcoin_block = db | ||
| .get_bitcoin_block(&tip_block_hash) | ||
| .await | ||
| .unwrap() | ||
| .unwrap(); | ||
|
|
||
| let tip_block_height = tip_bitcoin_block.block_height; | ||
|
|
||
| // This is what the current signer thinks is the state of things. | ||
| let req_ctx = ReqContext { | ||
| chain_tip: BitcoinBlockRef { | ||
| block_hash: tip_block_hash, | ||
| block_height: tip_block_height, | ||
| }, |
There was a problem hiding this comment.
nit: doesn't matter much, but this can be simplified to
let chain_tip = db
.get_bitcoin_canonical_chain_tip_ref()
.await
.unwrap()
.unwrap();
// This is what the current signer thinks is the state of things.
let req_ctx = ReqContext {
chain_tip,
...| let mut setup = | ||
| TestSweepSetup2::new_setup(test_signer_set.clone(), &faucet, &[deposit_amounts]); | ||
|
|
||
| setup.submit_sweep_tx(rpc, faucet, true); |
There was a problem hiding this comment.
Hmmm. So I think this means that the sweep transaction sweeping out the withdrawal gets confirmed. The store_sweep_tx function is supposed to store the sweep data in the right tables; I don't think that it does right now but will after #1359. When it does, the associated RejectWithdrawalV1 struct must fail validation. That's basically the double spend check from #478. Since this is the happy path, it's best to not submit the sweep tx using this function, moreover, I think store_sweep_tx panics if there is no sweep tx.
So for most of these tests we shouldn't submit a sweep transaction and store it in the database unless we want to test validation of a swept withdrawal request, which is the "The de-pegging check" from #478 (it used to say "The double spend check").
| let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; | ||
| match validation_result.unwrap_err() { | ||
| Error::WithdrawalRejectValidation(ref err) => { | ||
| assert_eq!(err.error, WithdrawalRejectErrorMsg::RequestNotFinal) | ||
| } | ||
| err => panic!("unexpected error during validation {err}"), | ||
| } | ||
|
|
There was a problem hiding this comment.
After this we should generate one more block and check that things validate then.
Description
Closes: #478
Changes
Testing Information
Checklist: