updatespendtx: merge signatures, don't overwrite the PSBT entirely

[darosior]

It would greatly improve the UX in the GUI (because it wouldn't discard existing signatures).

[darosior]

Some clarifications about this. The updatespendtx RPC takes a PSBT of a Spend transaction to update the one in DB. If there is none in DB it stores it, otherwise it overwrites it.

Instead of overwriting it, it would be preferable to query the in-DB PSBT, add to it the signatures from the new PSBT and then store it. Make sure to use revault_tx's add_signature() API which will check the validity of the signatures.

[Zshan0]

Hey, To update the progress, I'll list out the steps that I planned on taking

• Store the transaction that is being fetched by calling the function db_spend_transaction.
• If it exists then merge the signatures
• Pass in the new transaction to db_update_spend which is already being done

Although the suggested way was to call add_signature(), after a little exploration I found db_txs_merge_sigs and revault_txs_merge_sigs present in database/actions.rs, I can't directly use them because

1. The functions take transactions of the same type. And from what I understand, we will have the parameter transaction of type SpendTransaction whereas the transaction fetched is of the type DbSpendTransaction.
2. Both the functions are private.

I haven't checked if there is any way of typecasting SpendTransaction to DbSpendTransaction, if there is please let me know. In my opinion, we should use the pre-existing functionality to merge the signatures of two transactions rather than writing the functionality again in update_spend_tx.

[darosior]

the steps that I planned on taking

Sounds good to me.

Although the suggested way was to call add_signature(), after a little exploration I found db_txs_merge_sigs and revault_txs_merge_sigs [...] In my opinion, we should use the pre-existing functionality to merge the signatures of two transactions rather than [...]

First of all, note how those two functions you talk about trust their inputs. For instance:

revaultd/src/database/actions.rs

Line 769 in 457ff85

let sig = secp256k1::Signature::from_der(&sig[..sig.len() - 1]).expect("From DB");

That's because they are internal helpers working with data internal to the DB. Using such functions with inputs from the RPC would mean that we'd have to sanitize inputs first, and the easiest way of doing that is to use the add_signature API from revault_tx. Therefore we'd just be doing the work twice.

Secondly, those helpers are specific to presigned transactions (Unvault, Cancel and bother Emergency) which are managed separately from the Spend transactions in database:

revaultd/src/database/schema.rs

Lines 74 to 123 in 457ff85

	/* This stores transactions we presign:
	* - Emergency (only for stakeholders)
	* - Unvault
	* - Cancel
	* - Unvault Emergency (only for stakeholders)
	*/
	CREATE TABLE presigned_transactions (
	id INTEGER PRIMARY KEY NOT NULL,
	vault_id INTEGER NOT NULL,
	type INTEGER NOT NULL,
	psbt BLOB UNIQUE NOT NULL,
	txid BLOB UNIQUE NOT NULL,
	fullysigned BOOLEAN NOT NULL CHECK (fullysigned IN (0,1)),
	FOREIGN KEY (vault_id) REFERENCES vaults (id)
	ON UPDATE RESTRICT
	ON DELETE RESTRICT
	);
	/* A bridge between the Unvault transactions a Spend transaction
	* may refer and the possible Spend transactions an Unvault one
	* may be associated with.
	*/
	CREATE TABLE spend_inputs (
	id INTEGER PRIMARY KEY NOT NULL,
	unvault_id INTEGER NOT NULL,
	spend_id INTEGER NOT NULL,
	FOREIGN KEY (unvault_id) REFERENCES presigned_transactions (id)
	ON UPDATE RESTRICT
	ON DELETE RESTRICT,
	FOREIGN KEY (spend_id) REFERENCES spend_transactions (id)
	ON UPDATE RESTRICT
	ON DELETE CASCADE
	);
	/* This stores Spend transactions we created. A txid column is there to
	* ease research.
	* The 'broadcasted' column indicates wether a Spend transaction is:
	* - Not elligible for broadcast (NULL)
	* - Waiting to be broadcasted (0)
	* - Already broadcasted (1)
	* The 'has_priority' column indicates wether a Spend would automatically
	* be CPFPed if not confirmed in the first block after broadcast
	*/
	CREATE TABLE spend_transactions (
	id INTEGER PRIMARY KEY NOT NULL,
	psbt BLOB UNIQUE NOT NULL,
	txid BLOB UNIQUE NOT NULL,
	broadcasted BOOLEAN CHECK (broadcasted IN (NULL, 0,1)),
	has_priority BOOLEAN NOT NULL CHECK (has_priority IN (0,1)) DEFAULT 0
	);

Lastly, i'd like to get rid of these ugly hacks that db_txs_merge_sigs and friends are. But that's part of a big undertaking (#368).

[Zshan0]

Hey, continuing on this issue, I encountered more problem. Before I enumerate them, I thought I'll go through the process. First, I looked at the source code of set_spend_tx for inspiration and was able to get the list of signatures present in the transaction that is being fetched from the database, namely old_tx.

let signatures: Vec<BTreeMap<BitcoinPubKey, Vec<u8>>> = old_tx
    .unwrap()
    .psbt
    .psbt()
    .inputs
    .iter()
    .map(|i| i.partial_sigs.clone())
    .collect();

Then I iterate over them and then add the signature to spend_tx transaction. After that, I will pass the newly updated transaction to db_update_spend. In set_spend_tx, it expects "the signature was already there" error while adding signatures. If there is an overlap of signatures, this will remain how it is.

But do I have to also any other expected error that the system could encounter while adding the new signatures?

If it's more convenient, I can create a PR and then there will be more context to what I'm referring to.

[darosior]

I think set_spend_tx is a good source of inspiration, what we do there to sanity check the sigs is what we should do here to add them:

revaultd/src/commands/mod.rs

Lines 1245 to 1247 in 457ff85

	// Sanity check the Spend transaction is actually valid before announcing
	// it. revault_tx already implements the signature checks so don't duplicate
	// the logic and re-add the signatures to the PSBT.

In set_spend_tx, it expects "the signature was already there" error while adding signatures

It doesn't expect an error, it asserts that add_signature did not return None. It must never do so because the signature was already there when we added it.

[Zshan0]

Since I'm going to use add_signature to merge, I will have to call it on either old_tx or spend_tx to merge. I believe that I can't do so on the one that has been fetched from the database, but it is possible on spend_tx which has been passed as the argument. This would require it to be mutable. This will require changes in jsonrpc/api.rs. I don't see elsewhere in the API where &mut is being used So I'm not sure if this is the right direction, Is this change okay? or is there some other alternative that I can take?

revaultd/src/jsonrpc/api.rs

Lines 441 to 448 in b310208

	fn updatespendtx(
	&self,
	meta: Self::Metadata,
	spend_tx: SpendTransaction,
	) -> jsonrpc_core::Result<serde_json::Value> {
	meta.daemon_control.update_spend_tx(spend_tx)?;
	Ok(json!({}))
	}

[darosior]

I believe that I can't do so on the one that has been fetched from the database

Why wouldn't you?

is there some other alternative that I can take?

As previously stated, i think it should be done on the PSBT from the DB. It just sounds "safer".

[darosior]

This was fixed by #391.