Merging in update_spend_tx.

[Zshan0]

Currently, update_spend_tx over-writes the partial signatures that are present in the database with the signatures of the given spend_tx. To avoid this, the suggested change in #312 was to merge the older and the newer signatures together and save it in the database.

The corresponding test and the API documentation have been updated with the same

[Zshan0]

Hey, I noticed that I am still passing spend_tx to db_update_spend, but it must be unwrapped_tx. I was unable to find a db_update_spend version that takes DbSpendTransaction. Should I make one?

[darosior]

You could use the psbt member of old_tx if that's a DbSpendTransaction:

revaultd/src/database/schema.rs

Lines 180 to 188 in b310208

	/// A row in the "spend_transactions" table
	#[derive(Debug, PartialEq)]
	pub struct DbSpendTransaction {
	pub id: i64,
	pub psbt: SpendTransaction,
	pub broadcasted: Option<bool>,
	pub has_priority: bool,
	// txid is intentionally not there as it's already part of the psbt
	}

[darosior]

Thank you for the pull request. Since you are a new contributor, i'm very nit-picky and trying to comment on every small thing i notice. Don't worry, i won't do that every time and especially not for larger PRs but i think it's useful to increase communication a bit at first when someone joins the project. :-)

First a meta comment. A PR description like "as mentioned in issue #XXX" adds another indirection for an observer (which might well be us in 6 months) to get the context for a changeset. It's better to give a short (or not) summary of the rationale for this patch in the PR description. For instance here: "the current behaviour of updatespendtx would replace existing signatures if they are not present in the given Spend transaction. Instead of discarding signatures that are in database but not in the given transaction, merge the two PSBTs to keep all signatures in database.". Another motive for this is that we consider Github as transient, and the PR description is included in the git merge commit.
The same goes for commit messages. For a small PR like this one it's fine as it would be redundant to have it twice in the commit message and the PR description. But better for the rationale to be redundant than absent.

Now, on the patch. The approach is good. And i think the code is right. I've left a few things that should be addressed but it goes in the right direction.
However, a new feature should always come with a test that exercises it. Here, a new functional test_updatespendtx_merge test would be appropriate.
You should also update the API documentation in doc/API.md since it is a description of what the user should expect from this command. Just a small line saying signatures would be merged is enough.

[Zshan0]

To test if the merge is happening I have planned the following steps:

1. Create the same transaction spend_tx as created in test_spendtx_management
2. Push it to the database the first time using man.rpc.updatespendtx(spend_tx)
3. Obtain the corresponding psbt by deserializing spend_tx
4. Remove a signature from the only input of spend_tx, i.e. psbt.tx.vin[0]
5. Serialize it and call updatespendtx on new_spend_tx
6. Fetch it from the backend and assert if the transaction still has all the signatures

I'm kind of stuck on step 4 since I'm not sure how I can access the signatures from a CTxIn object, is there a simple way to do so?

[darosior]

The signature isn't part of a `CTxIn`, it's in the `PartiallySignedInput`: https://github.com/revault/revaultd/blob/c928b14e79d9f2b7ff2cdea22ba2df9049890d02/tests/test_framework/serializations.py#L602. But i think you can do something simpler. How about having 3 managers and: - Getting a Spend with `getspendtx`. Copying it in psbt_a and psbt_b. - Signing psbt_a with the first manager and calling `updatespendtx` with the signed transaction. - Signing psbt_b with the two other managers and calling `updatespendtx` with the signed transaction. - Querying the Spend using `listspendtx` and asserting that the transaction has 3 signatures (or is fully signed if you set a threshold of 3 for the managers). Also, note you'll need to rebase on master as the latest merges surely will create some hidden conflicts. ------- Original Message ------- Le mardi 17 mai 2022 à 5:48 PM, Zeeshan Ahmed ***@***.***> a écrit :

…

To test if the merge is happening I have planned the following steps: - Create the same transaction spend_tx as created in test_spendtx_management - Push it to the database the first time using man.rpc.updatespendtx(spend_tx) - Obtain the corresponding psbt by deserializing spend_tx - Remove a signature from the only input of spend_tx, i.e. psbt.tx.vin[0] - Serialize it and call updatespendtx on new_spend_tx - Fetch it from the backend and assert if the transaction still has all the signatures I'm kind of stuck on step 4 since I'm not sure how I can access the signatures from a CTxIn object, is there a simple way to do so? — Reply to this email directly, [view it on GitHub](#391 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F437JZBGF6JUBCTQCTVKO5VZANCNFSM5UPFQWJQ). You are receiving this because you commented.Message ID: ***@***.***>

[darosior]

Nice.

ACK 0e80faf

[darosior]

nit: you can use this syntax to avoid repetition:

CommandError::SpendFeerateTooLow(_, _) | CommandError::SpendSignaturesUpdate(_) => ErrorCode::INVALID_PARAMS,

Note: "nit" review comment are "Not Important Things". We use them to convey or discuss something, without it being an explicit request to the PR author to address it. In other words: feel free to address, or not.

[darosior]

nit: you can just use the mans method :)

revaultd/tests/test_framework/revault_network.py

Lines 420 to 432 in b47212b

	def mans(self):
	return self.stkman_wallets + self.man_wallets
	def stks(self):
	return self.stkman_wallets + self.stk_wallets
	def participants(self):
	return self.stkman_wallets + self.stk_wallets + self.man_wallets
	def man(self, n):
	"""Get the {n}th manager (including the stakeholder-managers first)"""
	mans = self.stkman_wallets + self.man_wallets
	return mans[n]