fix: check recipient against blocklist client in withdrawals

[MCJOHN974]

Description

Closes: #1415

Changes

• Now we check recipient against blocklist client in withdrawals, not sender as it was before.
• Improved a bit error handling

Testing Information

• I added no new tests but as I see same function for deposits also don't have such a test. Considering that this function is pretty small, simple, and can't lead to critical bug we probably ok with that

Checklist:

• I have performed a self-review of my code
• My changes generate no new warnings
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[MCJOHN974]

I'm not really happy with this naming but I don't really see which outpoint we can attach to withdrawals, thus it is okish I think

[cylewitruk]

For deposits it's not necessarily the "outpoint" itself that we're interested in, it's just that the outpoint is the unique identifier of the deposit request; for withdrawals, it's the request_id (+ its block_hash to identify it across forks). I might opt to name these WithdrawalBitcoinAddress and DepositBitcoinAddress or something along those lines, and have the withdrawal version accept its request id and block hash.

[MCJOHN974]

Btw, about issue itself. I can see why we checking recipient is more important then sender:
transferring funds to blocklisted address can put transferrer in trouble, while locking & burning coins from blocklisted address probably is not so dangerous

But if my intuition is correct why we do not check recipient in deposits? Because we mint coins for them and not directly transfer?

And very final question: why don't we want to check both sender and recipient agains blocklist?

pinging @djordon as author of issue

[cylewitruk]

Btw, about issue itself. I can see why we checking recipient is more important then sender: transferring funds to blocklisted address can put transferrer in trouble, while locking & burning coins from blocklisted address probably is not so dangerous

But if my intuition is correct why we do not check recipient in deposits? Because we mint coins for them and not directly transfer?

And very final question: why don't we want to check both sender and recipient agains blocklist?

The sender/recipient is always a stacks/bitcoin address pair and I'm not sure that there are any sanctions for Stacks addresses (at least in OFAC or the EU sanctions list). Nor am I sure if Chainalysis does any specific forensic/clustering analysis on the Stacks blockchain that would enable their screening service to properly screen Stacks addresses?

However, the blocklist client is intended to be able to be swapped out for a custom implementation, which may include a signer's own risk analysis/database supporting STX addresses, so I do think that we should eventually be sending the request for both and that our implementation simply filters out Stacks addresses for now. But in practice, all of the signers are using Chainalysis at the moment, so there's no direct demand for supporting STX addresses.

[cylewitruk]

I know we don't have them today or for deposits either, but I wouldn't be sad if a test or two popped up ensuring that this error is actually returned if the receiver pubkey is bad/network mismatch/etc.

[MCJOHN974]

However, the blocklist client is intended to be able to be swapped out for a custom implementation, which may include a signer's own risk analysis/database supporting STX addresses, so I do think that we should eventually be sending the request for both and that our implementation simply filters out Stacks addresses for now. But in practice, all of the signers are using Chainalysis at the moment, so there's no direct demand for supporting STX addresses.

Until our canonical blocklist implementation returns ok on addresses it don't know I think nothing stops us to test both stacks and bitcoin addresses now. However, delaying it can lead to forgetting, thus I'd suggest to implement it now

[MCJOHN974]

Hey, @cylewitruk, I added some test for both deposits and withdrawals. It is not isolating only this can_accept function, but it look nice I think

[MCJOHN974]

Tbh a bit annoying that get_withdrawal_request_signer_votes and get_deposit_signer_decisions are different =)

[cylewitruk]

There is get_deposit_request_signer_votes(), is that maybe what you meant to use?

[MCJOHN974]

Originally it was test for deposits which I rebranded to both deposits and withdrawals. It was using get_deposit_signer_decisions, and get_deposit_signers. I keep them and tried to find the closest signer analogues, but get_withdrawal_signer_decisions doesn't exist. But anyway I think that this functions are fine and we extract what we want in both deposits and withdrawals

[djordon]

Btw, about issue itself. I can see why we checking recipient is more important then sender: transferring funds to blocklisted address can put transferrer in trouble, while locking & burning coins from blocklisted address probably is not so dangerous

But if my intuition is correct why we do not check recipient in deposits? Because we mint coins for them and not directly transfer?

And very final question: why don't we want to check both sender and recipient agains blocklist?

pinging @djordon as author of issue

What Cyle wrote in #1428 (comment) is spot on. We don't check the Stacks side because we do not think that the signers need to. If that changed then we may have to change the code here.