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

txscript: return multisig signing error for no-op #3167

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

norwnd
Copy link
Contributor

@norwnd norwnd commented Jul 26, 2023

Just something I noticed when fiddling with multisigs,

if that change makes sense (after feedback) I'll finish this draft.

All Submissions:

  • Submission follows the Code Contribution Guidelines
  • There are not any other open Pull Requests for the same update/change
  • Commit messages are formatted according to Model Git Commit Messages
  • All changes are compliant with the latest version of Go and the one prior to it
  • The code being submitted is commented according to the Code Documentation and Commenting section of the Code Contribution Guidelines
  • Any new logging statements use an appropriate subsystem and logging level
  • Code has been formatted with go fmt
  • Running go test does not fail any tests or report any vet issues
  • Running golint does not report any new issues that did not already exist

New Feature Submissions:

  • Code is accompanied by tests which exercise both the positive and negative (error paths) conditions (if applicable)

Bug Fixes:

  • Code is accompanied by new tests which trigger the bug being fixed to prevent regressions

Changes to Core Features:

  • An explanation of what the changes do and why they should be included is provided
  • Code is accompanied by updates to tests and/or new tests for the core changes (if applicable)

Comment on lines -121 to +124
// possible. It returns the generated script and a boolean if the script
// fulfills the contract (i.e. nrequired signatures are provided). Since it is
// arguably legal to not be able to sign any of the outputs, no error is
// returned.
// possible. It returns the generated script if 1 or more signature was provided,
// otherwise it returns detailed error (while some parts of that error message
// are expected artifacts of execution others might be useful when debugging
// signing issues).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is
// arguably legal to not be able to sign any of the outputs, no error is
// returned.

Pointing out this comment, I might be missing something, but I don't see why somebody would try to sign multisig transaction unless he expects at least 1 signature to succeed.

@davecgh
Copy link
Member

davecgh commented Jul 26, 2023

I don't think this can be changed without causing a major breaking change to the wallet RPC server. It definitely can't be changed without also cause a major version bump to the txscript module at a minimum.

Currently, the RPC can be used to determine if a multisig transaction is fully signed by just attempting to sign it and checking the "complete" flag in the result. That would no longer be possible with the proposed change.

Personally, I don't think this should be changed because it would cause a lot of churn for very little gain and also would break the aforementioned which I'm pretty sure various things depend on.

That said, since @jrick is the primary person who works on wallet, his feedback would be good to have on this.

@jrick
Copy link
Member

jrick commented Jul 26, 2023

There's a patch on the dcrwallet issue that probably solves this better without making this incompatible change.

@norwnd
Copy link
Contributor Author

norwnd commented Jul 27, 2023

Personally, I don't think this should be changed because it would cause a lot of churn for very little gain and also would break the aforementioned which I'm pretty sure various things depend on.

Yeah. I thought this might not be worth the trouble ... still I hope pointing the issue out doesn't hurt, somebody might find this discovery valuable later on.

Feel free to close, but I'd keep it open until we find resolution on dcrwallet side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants