Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Validate bitcoin transaction sign requests #286

Closed
netrome opened this issue Jul 3, 2024 · 2 comments · Fixed by #930
Closed

[Feature]: Validate bitcoin transaction sign requests #286

netrome opened this issue Jul 3, 2024 · 2 comments · Fixed by #930
Assignees
Labels
half sprint demo sbtc signer binary The sBTC Bootstrap Signer.

Comments

@netrome
Copy link
Contributor

netrome commented Jul 3, 2024

Context

In #285 the logic for responding to bitcoin transaction sign requests were introduced. In that PR, the signers accept all requests. However, we should ensure that the signers only respond to valid sign request. A sign request is considered valid for a signer to sign if:

  1. All deposit requests consumed by the bitcoin transaction are accepted by the signer.
  2. All withdraw requests fulfilled by the bitcoin transaction are accepted by the signer.
  3. The apportioned transaction fee for each request does not exceed any max_fee.
  4. Each deposit request input has an associated amount that is greater than their assessed fee.
  5. There is at least 3 blocks left before the depositor can reclaim their funds.

Definition of done

The transaction signer only acknowledges valid bitcoin transactions.

@netrome netrome added the sbtc signer binary The sBTC Bootstrap Signer. label Jul 3, 2024
@netrome netrome added this to sBTC Jul 3, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in sBTC Jul 3, 2024
@netrome netrome moved this from Needs Triage to Todo in sBTC Jul 3, 2024
@AshtonStephens AshtonStephens added this to the sBTC Code Complete milestone Aug 15, 2024
@netrome netrome added the good first issue Good for newcomers label Sep 3, 2024
@djordon djordon removed the good first issue Good for newcomers label Sep 21, 2024
@cylewitruk
Copy link
Member

cylewitruk commented Oct 22, 2024

All deposit requests consumed by the bitcoin transaction are accepted by the signer.
All withdraw requests fulfilled by the bitcoin transaction are accepted by the signer.

@djordon hm, so based on the above, the signer not liking a single deposit/withdrawal (which maybe was accepted by all of the other signers) will cause the signer to withhold its vote on the entire sweep transaction, right? Feels kinda like the signer should sign anyway if they're part of the minority vote, otherwise if this happens continuously and sporadically it could deadlock the signers since I'm guessing they won't approve any re-attempts either? Not sure if I'm making any sense...

@djordon
Copy link
Collaborator

djordon commented Oct 22, 2024

All deposit requests consumed by the bitcoin transaction are accepted by the signer.
All withdraw requests fulfilled by the bitcoin transaction are accepted by the signer.

@djordon hm, so based on the above, the signer not liking a single deposit/withdrawal (which maybe was accepted by all of the other signers) will cause the signer to withhold its vote on the entire sweep transaction, right?

Yeah that is correct.

Feels kinda like the signer should sign anyway if they're part of the minority vote, otherwise if this happens continuously and sporadically it could deadlock the signers since I'm guessing they won't approve any re-attempts either? Not sure if I'm making any sense...

I think I understand what your saying but I don't think that the signers should sign anyway. This is based on a couple reasons:

  1. A signer is under no obligation to sign for a transaction that it disagrees with;
  2. The coordinator knows (1) and constructs transactions that it thinks will be accepted by enough signers to generate a signature.

So, I don't think that there are any of the risks that you describe. But if we missed something, the fix is most likely with changing the coordinator's behavior.

@github-project-automation github-project-automation bot moved this from In Review to Done in sBTC Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
half sprint demo sbtc signer binary The sBTC Bootstrap Signer.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants