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

Mutlisignature protocol amelioration #120

Open
MisterTicot opened this issue May 21, 2018 · 8 comments
Open

Mutlisignature protocol amelioration #120

MisterTicot opened this issue May 21, 2018 · 8 comments
Labels
CAP Represents an issue that requires a CAP. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.

Comments

@MisterTicot
Copy link
Contributor

Current behavior

When a transaction is signed by more legit signers than required, it will be rejected with 'tx_bad_auth_extra' error.

Proposed behavior

The transaction should be validated and each legit signer should be recorded into the blockchain; Or at least the transaction should get validaded.

Rationale

There's two distinct propositions here:

  1. A transaction with enough valid signatures should always be validated:
  • First of all rejecting valid transactions is a non-sense
  • Rejecting transaction with more-than-needed signers force wallets willing to implement multisignature to add routines to check that there's not "too much" signers and to strip them out. This routine could be implemented at stellar-core level, saving wallets implementors from this redundant task and making them more likely to implement it right in the first place.
  1. Additional signers should be recorder in the block chain:
  • There's use cases where it does matters who signed, beyond the fact that the threshold have been met. For instance: voting system, legal contracts, proof of presence. Those are commons use of blockchain systems that would easily and cheaply get supported by Stellar if this proposal get accepted. Supporting those use cases would be beneficial for Stellar and its ecosystem.
@cesarmak
Copy link

cesarmak commented May 25, 2018

This is particular true when a transaction has multiple operations (and/or using channels).

Example

P has 3 signers: P, Q, R (each with weight=1)
For P, all thresholds = 2 (i.e., 2 signers needed)

  1. R conducts the tx (R as a channel)
  2. add Operation: P pays Z, 1XLM
  3. add Operation: Q pays Z, 1XLM
  4. sign with P, Q, R
  5. submit tx -->tx_bad_auth_extra

I can't think of actual use case of the above scenario yet, but it seems the system deemed the 1st operation invalid.

@MonsieurNicolas
Copy link
Contributor

Reason we did this was because of the quadratic nature of signature verification (which is the most CPU intensive thing we deal with before accepting transactions in consensus - the hints help but they don't help making the worst case). I agree this may be more burden than what it's worth.
It may make sense to reconsider this the next time we look into changing the fee structure: I think that there should still be an incentive to submit minimal transactions to the network.

@vogel
Copy link
Contributor

vogel commented Jun 5, 2018

@cesarmak your example should get executed if only P and Q signed this transaction.

@cesarmak
Copy link

cesarmak commented Jun 6, 2018

@vogel yes, I've tested a bit on the marginal case; just wanted to point out that some possible scenarios could be rejected.

So, is it the proper way to query Horizon on the Account Endpoint, and do a weight checking with the "threshold" & "signers", before trying to submit the transaction?

@vogel
Copy link
Contributor

vogel commented Jun 7, 2018

Yes, that is the proper way.

@theaeolianmachine
Copy link
Contributor

@MonsieurNicolas is it worth reviving this discussion? I agree it's a weird behavior that affects usability, but it feels mildly painful as opposed to detrimental (especially if changing the behavior would hurt performance a good bit). @graydon do you have any thoughts?

If we'd be open to discussing this further, I'd like to push this to be an official draft and put it on the docket.

@theaeolianmachine theaeolianmachine added CAP Represents an issue that requires a CAP. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process. labels Mar 12, 2019
@MonsieurNicolas
Copy link
Contributor

I have nothing else to add to this thread at this point

@theaeolianmachine
Copy link
Contributor

@MonsieurNicolas do you mean "yes, I think this should be revisited but I don't have the bandwidth to look into it/write a CAP" or "no, I don't think the cost of implementation is worth the benefits, and we should close this out and do a better job documenting this."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CAP Represents an issue that requires a CAP. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.
Projects
None yet
Development

No branches or pull requests

6 participants
@vogel @theaeolianmachine @MonsieurNicolas @cesarmak @MisterTicot and others