bug: change_utxo not always present

[marcellorigotti]

Add logic to btc_utxos_balance rpc

Pull Request

Closes: PRO-1716, PRO-1739

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have written sufficient tests.
• I have written and tested required migrations.
• I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

[codecov]

Codecov Report

Attention: Patch coverage is 45.23810% with 23 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (09f7507) to head (94c44cc).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/runtime/src/lib.rs	8%	0 Missing and 23 partials ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5340    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        494     494            
  Lines      86142   86037   -105     
  Branches   86142   86037   -105     
======================================
- Hits       61164   60976   -188     
- Misses     22245   22298    +53     
- Partials    2733    2763    +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[martin-chainflip]

That is a weird message and also shouldn't panic in debug? I think we should just log "Witnessed BTC egress without change UTXO"

[ramizhasan111]

could just directly be

 let change_output = batch_transfer.bitcoin_transaction.outputs.last().unwrap();

[ramizhasan111]

small comment, otherwise looks good (apart from Martin's comment).

[dandanlen]

We should also consider checking against the previous_pubkey: imagine we send a tx before but witness it after a rotation/key handover.

Also: Maybe we should do this check for each of the outputs?

For example imagine someone sends funds to the vault (I don't know why...)

ie we could do:

for output in outputs {
	if [vault.current_key, vault.previous_key].contains(output.address) {
		// add to utxos
	}
}

@martin-chainflip ?

[dandanlen]

Alternatively: we encode the change amount directly in the BatchTransfer type, then we don't need to parse anything from the utxos.

(ie. we change the type to:

pub struct BatchTransfer {
	pub bitcoin_transaction: BitcoinTransaction,
	// Use a struct:
	pub change_utxo_details: (
		[u8; 32], VOut, TxId, Amount
	),
}

[martin-chainflip]

i think Dan's first suggestions is best.

[ramizhasan111]

I dont understand why we need to check all the outputs? The change utxo will always be the last output due to how the tx is constructed?

[dandanlen]

Anyone can open a channel sending funds to the vault. (could be by accident, could be on purpose: maybe we want to top up the vault?)

[ramizhasan111]

But why would that change the order of the outputs of the tx? When we create a btc tx, we select some utxos from the vault, and create output utxos (transfers, and possibly change utxo appended at the end), and save this output utxo list in the BitcoinTransaction type. So when the tx is successfully signed, we then take the change utxo and add it to the available utxo list which is what we are doing here. I dont see how someone sending funds to the vault is relevant here.

[martin-chainflip]

Imagine I request a swap with the BTC vault as the destination address. Then we construct a transaction with two outputs, both of which will go into the vault. Pretty theoretical example, but this code clearly does the right thing, whereas just checking the last output adds a lot of unnecessary assumptions that might break in the future.

[ramizhasan111]

In that theoretical example, i would say that the right thing to do is not egress at all because you are basically taking a utxo from the vault, splitting it in two and sending it back to the vault (which is pointless if not actually harmful because we are splitting utxos) and so instead of allowing the egress to happen and deal with it here in an indirect way, I would just not egress.

The code here is specifically to add the change utxo to the available utxo list and is tightly coupled with the code of creating btc tx. I dont think we are making assumptions here because we construct the btc tx a certain way (putting the change utxo at the end) and until we change how we construct btc tx, this should always be the case.

[dandanlen]

Instead of doing all this it might be nicer to just store the change utxo somewhere else on-chain temporarily and then move it back to the utxo set when it's ready...

This also would avoid needing to reconstruct it, removing another source of assumptions.

(can wait for another time though)