fix: emily validate deposits

[Jiloc]

Description

Closes: #1116,

Changes

• add a new transaction_hex mandatory field to the create_deposit endpoint
• add validation on all request fields and the new one
• add validation to check if the raw transaction data is an sbtc transaction and its content corresponds to the txid, deposit and reclaim scripts
• extract deposit amount from the tx - previously we were hardcoding a 0.
• add validation to check if the deposit amount is withint the per-deposit limits:
  • the validation is simple by design. We want to avoid requests that are clearly malicious to be accepted by Emily, but the refined validation (e.g. amount - paid fee > dust fee) are still expected from the signers.
• save recipients with their real string value instead of their hex representation. e.g. SP5ZH8581RX20N9MJXZ0V7G0QKCBKGDKM4EK3700 insead of 05160bf8a0a80e3a205534977e0d9e00bcd8b9c1b3a1

Testing Information

• added real txs fixtures and unit tests for the validation
• modified tx_setup to create transactions with mulitple deposits
• updated unit and integration tests

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

[Jiloc]

We'll keep this PR in Draft till after the rotate-key release

[djordon]

Looks good.

Left a suggestion. I'm just refraining from approving until after key rotation work.

[djordon]

nit: you can remove this if you move up the validate_tx call, which I think also has a dedicated error message for this check:

Suggested change

	let amount = tx
	.tx_out(self.bitcoin_tx_output_index as usize)
	.map_err(|_| {
	Error::HttpRequest(
	StatusCode::BAD_REQUEST,
	"invalid bitcoin_output_index".to_string(),
	)
	})?
	.value
	.to_sat();
	let deposit_info = deposit_req
	.validate_tx(&tx)
	.map_err(|e| Error::HttpRequest(StatusCode::BAD_REQUEST, e.to_string()))?;

Then below just do

...
if deposit_info.amount < min {
    ...

[Jiloc]

I noticed this, but I preferred the current approach because I wanted the deposit's amount to be validated before the transaction matching, and I found the error message for the existing validation (could not get outpoint f75cb869600c6a75ab90c872435da38d54d53c27afe5e03ac7dedae7822958de:10 from BTC transaction: invalid output index) too verbose. Said that, I am aware that my motivations are just a small subjective preference, so I am open to change if you think the other approach would be clearer!

[fabergat]

The CDK part looks good to me

[matteojug]

I'm a bit worried about validating the limits here (other than the dust), as there's a time difference between when the bridge checks for the limits and when Emily does. And in particular, if the request is not on emily the bridge cannot offer a recovery via reclaim. So maybe it's better to not enforce the limits checks in here.

[Jiloc]

The main idea was that it would also validate requests that won't arrive from the bridge. if they arrive from the bridge and it's properly handling the limits, this should never happen. With the exception of a very unfortunate race condition of us changing the limits while a deposit is being validated. I am find with removing it if you still think it's safer though

[matteojug]

I'm wondering if we want to keep the dust limit check or not (I know previously I said so, sorry 🙏 ): it's kinda a constant of the system so I would be inclined to keep it, but OTOH there's the usual: what if someone else (the bridge, some external wallet) doesn't enforce it? Then we are back to the usual more work for the signers but users can reclaim easily thingy

[Jiloc]

🤣 no worries, it makes sense to me. Any kind of enforcement on Emily's side makes sense only if the create deposit call could be sent and validated before the bitcoin TX is sent, but given that it's not the case..