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

Tx Structure #669

Closed
7 tasks done
ebuchman opened this issue Mar 20, 2018 · 16 comments
Closed
7 tasks done

Tx Structure #669

ebuchman opened this issue Mar 20, 2018 · 16 comments
Assignees

Comments

@ebuchman
Copy link
Member

ebuchman commented Mar 20, 2018

We discussed

type Msg interface {
	Type() string
	Get(key interface{}) (value interface{})
	ValidateBasic() Error
	GetSigners() []Address
}

type StdSignature struct {
	PubKey   crypto.PubKey `json:"pub_key"` // optional
	Sequence int64         `json:"sequence"`
        Number int64           `json:"number"`

	crypto.Signature `json:"signature"`
}

type StdTx struct {
	Msgs       []Msg          `json:"msgs"`
	Fee        StdFee         `json:"fee"`
	Signatures []StdSignature `json:"signatures"`
}

type StdSignDoc struct {
	ChainID string
	Fee     StdFee
	Msgs    []Msg
        Memo    []byte

        // signer sequence and number
	Sequence int64
        Number   int64
}

One thing in my notes was to have Memo in each StdSignature but I'm not sure this makes sense since only one signer pays the fee.

@cwgoes
Copy link
Contributor

cwgoes commented Apr 6, 2018

If each signer has their own StdSignDoc, with their own account nonce - why does each StdSignDoc have a Fee and a Memo? Only one signer pays the fee. Should there be an additional FeePayer boolean field, perhaps?

Does each signer get to set their own Memo / what are memos used for / are they stored (do we need to think about gas costs for long memos, or memo size limitations)?

@cwgoes
Copy link
Contributor

cwgoes commented Apr 6, 2018

Per discussion in person with @sunnya97 - can we just allow multiple signers to pay fees on the same transaction? This provides more options - e.g. if three people split payment for dinner in the same transaction, they can also split the transaction fee - and still allows for a single fee payer when desired.

@ebuchman
Copy link
Member Author

ebuchman commented Apr 6, 2018

Hmm.

Re Memo - the idea is each signer can sign some piece of meta data in there. It can get pruned out of the blockchain but we do need to consider cost.

Re multiple fee payers - I believe the goal was for fee payment checks to be as cheap as possible - ie only one account has to be loaded. The effect of multiple fee payers can be simulated by just having them pay back the one fee payer in outputs.

But you may be right that not every signer needs to sign the fee, just the fee payer.

@cwgoes
Copy link
Contributor

cwgoes commented Apr 6, 2018

Minimizing fee payment check cost makes sense.

Presumably Memos could just be hard-limited on size to a few hundred bytes - if used by e.g. an exchange to trace transactions, they'll probably be external reference identifiers anyways.

Perhaps non-fee-paying signers can just sign a fee of zero?

@sunnya97
Copy link
Member

sunnya97 commented Apr 6, 2018

Regarding the multiple fee payers, it's not like it's any more state reads, you're still going to have to load all the signer's accounts in order to check the signatures.

Also, you could just charge slightly more gas for having it split across more fee payers?

@rigelrozanski
Copy link
Contributor

moreover,

can we just allow multiple signers to pay fees on the same transaction?

Wasn't the point to make the fee check in checktx as lean as possible for spam prevention? Limiting the fee payer to one party was an attempt to minimize the spam attack surface I believe

@sunnya97
Copy link
Member

sunnya97 commented Apr 7, 2018

But you can just do the summing of the fees in the same for loop as checking the signatures. This has like such a negligible effect on the cost of CheckTx.

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Apr 7, 2018

the FIRST thing we do is check the signature for the fee - if this is bad then we immediately throw away the transaction before checking the signatures, so it's not as if a new attack scenario isn't opened up with multiple fee payers.. imagine the scenario where a spammer pays the fee with 1000 sigs, and only the last one is bad (but contains the majority of the fee) - I'm not sure on the relative cost on checkTx maybe it's negligible

@sunnya97
Copy link
Member

sunnya97 commented Apr 7, 2018

Yeah, but like the spammer could also just send you 1000 tx in which the first signature is invalid. The solution is the same, you should unpeer with a spammer spending too many tx that fail checkTx.

Also, how the current antehandler works right now, all the signatures are checked in CheckTx, so that way a spammer could just have enough fee payment in the first payer, and then cause CheckTx to fail by having the last signature check fail. Unless we want CheckTx to only check the fee payer's signature, and nothing else?

@rigelrozanski
Copy link
Contributor

hmmm interesting, yeah I wonder, maybe the anteHandler should only really deduct fees??? The client would then be expected to run validateBasic, and if they didn't their fees would just get eaten... This actually makes sense to me. It's fine if the transaction fails so long as enough fee has been paid out first... the problem only arises when there is significant computation which can arise BEFORE the fee has been deducted. I suppose, it would technically be safe to allow for multiple fee payers so long as each fee was deducted as they were checked, and there would be a minimum fee which any fee payer could pay (which is would be the computational cost of checking that account and deducting that individual fee)...... cool

@martindale
Copy link

martindale commented Apr 16, 2018

Moving this to backlog (i.e., "to do after launch") edit: tabling #670, continuing to work on this for launch.

@martindale martindale added this to the 1.0 Code Freeze milestone Apr 16, 2018
@cwgoes cwgoes removed their assignment Apr 17, 2018
@ebuchman
Copy link
Member Author

Shouldn't we handle this sooner than later ? Since all txs will be effected. At least:

  • Only sign your own nonce
  • AccountNumber (yes, for account pruning)

@sunnya97
Copy link
Member

I split this issue into multiple constituent issues. Added all the new issues to the main comment of this issue.

@ValarDragon
Copy link
Contributor

I know this particular point has been resolved, but just wanted to point out another option for the concern of the 1000th signer's fee being checked before deducting fees. We could do a thing where every full node has a cached random shuffle of numbers from 1-1000, and they iterate according to that shuffle. That way an attacker can't ensure that their malicious tx will take a long time to fail on every full node, since each full node will have a different random shuffle.

@rigelrozanski
Copy link
Contributor

I thought we resolved with only ever allowing for one fee payer for a multi-sig CC: @sunnya97 @jaekwon

@AdityaSripal
Copy link
Member

Closed in #1266

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

No branches or pull requests

7 participants