auth: Don't recalculate mempool fees for every msg signer, misc. cleanup #2376

ValarDragon · 2018-09-22T05:33:51Z

This PR begins improving the godocs for the auth module, and begins cleaning
up the Ante handler to (imo) improve efficiency and be safer.

Additionally we previously calculated if the fee was sufficient for the mempool
on every single signer. This is now refactored to be only done once, thereby being more efficient,
and to have a better logical flow. No changelog entry as mempool is new to this release.

This PR does another thing. It moves all checks before verifying any signatures, so that if something was wrong we didn't waste unnecessary time.

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

This PR begins improving the godocs for the auth module, and begins cleaning up the Ante handler. Additionally we previously calculated if the fee was sufficient for the tx on every single signer. This is now refactored to be more efficient, and have a better logical flow. No changelog entry as this is new to this release.

codecov · 2018-09-22T06:09:57Z

Codecov Report

Merging #2376 into develop will decrease coverage by 0.01%.
The diff coverage is 78.12%.

@@             Coverage Diff             @@
##           develop    #2376      +/-   ##
===========================================
- Coverage    63.54%   63.53%   -0.02%     
===========================================
  Files          120      120              
  Lines         7236     7250      +14     
===========================================
+ Hits          4598     4606       +8     
- Misses        2326     2333       +7     
+ Partials       312      311       -1

…ations

ValarDragon · 2018-09-22T06:30:42Z

x/auth/ante.go

-			// first sig pays the fees
-			// Can this function be moved outside of the loop?
-			if i == 0 && !fee.Amount.IsZero() {
-				newCtx.GasMeter().ConsumeGas(deductFeesCost, "deductFees")


I removed charging gas for deducting fees, because that's silly lol

ValarDragon · 2018-09-22T06:33:14Z

I just realized the diff for this is kind of hard to review, because I re-ordered things to be in more of (what I consider) to be a logical ordering.

To ease reviewing this, I'll try commenting where everything that was deleted was moved to

ValarDragon · 2018-09-22T06:33:40Z

x/auth/ante.go

@@ -36,13 +33,17 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			return ctx, sdk.ErrInternal("tx must be StdTx").Result(), true
 		}

-		// set the gas meter
-		if simulate {
-			newCtx = ctx.WithGasMeter(sdk.NewInfiniteGasMeter())


moved to setGasMeter method.

ValarDragon · 2018-09-22T06:34:15Z

x/auth/ante.go

@@ -66,64 +67,48 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 		if err != nil {
 			return newCtx, err.Result(), true
 		}
-
-		sigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice.


simply renamed to stdSigs to distinguish from it being the actual cryptographic signature as was previously implied.

ValarDragon · 2018-09-22T06:35:09Z

x/auth/ante.go

 		// charge gas for the memo
 		newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo")

-		// Get the sign bytes (requires all account & sequence numbers and the fee)
-		sequences := make([]int64, len(sigs))
-		accNums := make([]int64, len(sigs))


The need for these was removed by getting the accounts directly with getSignerAccounts, and getSignBytesList

ValarDragon · 2018-09-22T06:36:22Z

x/auth/ante.go

-
-			// first sig pays the fees
-			// Can this function be moved outside of the loop?
-			if i == 0 && !fee.Amount.IsZero() {


This code was moved outside of the for loop (right above it!)

ValarDragon · 2018-09-22T06:38:46Z

x/auth/ante.go

-				fck.addCollectedFees(newCtx, fee.Amount)
+		for i := 0; i < len(stdSigs); i++ {
+			// check signature, return account with incremented nonce
+			res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytesList[i], simulate)


refactored to have this take in the account for clarity. Additionally, it now has sequence / account number done separately in validateAccNumAndSequence. This means if theres an invalid accnum / seq num, that we don't any of the more expensive signature verifications.

alessio

ACK from me. Looks very good indeed.

alexanderbez

Thanks @ValarDragon -- just a minor remark, otherwise LGTM.

alexanderbez · 2018-09-24T12:53:08Z

x/auth/ante.go

-			signerAcc, res := processSig(newCtx, am, signerAddr, sig, signBytes, simulate)
+		// first sig pays the fees
+		if !stdTx.Fee.Amount.IsZero() {
+			signerAccs[0], res = deductFees(signerAccs[0], stdTx.Fee)


There is a FeePayer method on a sdk.Tx.

I don't see it?

// Transactions objects must fulfill the Tx type Tx interface { // Gets the Msg. GetMsgs() []Msg }

Oh its on StdTx. I don't think we should use that though, since we already have access to the signerAccs, and doesn't make sense to run that method to get it again. I'd be happy to just make feePayer := signerAccs[0] for code clarity though.

I think we should just remove the feepayer method entirely, as its currently unused. If we want we can refactor this fee deduction as used here into its own method (I think that makes sense as well)

In line of principle, it's not a bad idea to provide SDK clients with a convenience FeePayer method. Having said that, it's clearly unused thus +1 for the removal.

Yeah just remove it as it's not part of the interface (and use feePayer variable).

If you remove it, please mention it as a breaking change 👍

actually using a feepayer variable looks weirder, since we need to update the entry in the array. I've removed the fee payer method though, and added an additional comment for clarity.

alexanderbez · 2018-09-24T12:55:53Z

x/auth/ante.go

+}
+
+func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (
+	signatureBytesList [][]byte) {


Can we have this on the same line or move the arguments to the same line, took me a second to realize it was returning something?

jaekwon · 2018-09-25T04:51:42Z

x/auth/ante.go

-		} else {
-			newCtx = ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
+		// Ensure that the provided fees meet a minimum threshold for the validator, if this is a CheckTx.
+		// This is for mempool purposes, and thus is only ran on check tx.


for local/subject mempool purposes,
only "run" on ...

jaekwon · 2018-09-25T05:01:38Z

x/auth/ante.go

@@ -245,8 +241,9 @@ func consumeSignatureVerificationGas(meter sdk.GasMeter, pubkey crypto.PubKey) {
 }

 func adjustFeesByGas(fees sdk.Coins, gas int64) sdk.Coins {
-	gasCost := int64(float64(gas) * feeDeductionGasFactor)
+	gasCost := int64(float64(gas) * gasPrice)


we can't use float64().

jaekwon · 2018-09-25T05:23:50Z

x/auth/ante.go

@@ -245,8 +242,9 @@ func consumeSignatureVerificationGas(meter sdk.GasMeter, pubkey crypto.PubKey) {
 }

 func adjustFeesByGas(fees sdk.Coins, gas int64) sdk.Coins {
-	gasCost := int64(float64(gas) * feeDeductionGasFactor)
+	gasCost := gas / gasPrice


gasPrice is a misnomer, it's gasPerUnitCost.

ValarDragon · 2018-09-26T16:26:30Z

Jae and I had a conversation about changing how we adjust fees by gas, to ensure it works well in the int64 range when we move to nano-atoms. I'll write a follow-up issue describing that conversation. I believe that is fairly independent of this PR, and that we should go ahead and merge this.

resolved :)

alexanderbez · 2018-09-26T16:30:28Z

Yeah @ValarDragon im Ok with that 👌 Be sure to reference this issue in the new one I guess.

alexanderbez

utACK

cwgoes

utACK

ValarDragon added ready-for-review labels Sep 22, 2018

ValarDragon requested review from alessio and cwgoes September 22, 2018 05:33

ValarDragon requested review from ebuchman and rigelrozanski as code owners September 22, 2018 05:33

ValarDragon removed request for ebuchman and rigelrozanski September 22, 2018 05:34

ValarDragon force-pushed the dev/cleanup_auth branch from 4139680 to 68ee2eb Compare September 22, 2018 05:39

auth: Fix placement of fee deduction

8097c52

ValarDragon added 2 commits September 21, 2018 23:15

Verify all sequences and account numbers before any signature verific…

8f817af

…ations

Add changelog entry, plus no longer charge gas for charging fees

9332e4a

ValarDragon commented Sep 22, 2018

View reviewed changes

minor cleanup

199cfe7

alessio approved these changes Sep 22, 2018

View reviewed changes

alexanderbez previously requested changes Sep 24, 2018

View reviewed changes

ValarDragon added 2 commits September 24, 2018 10:26

Make function signature more clear

a23d454

Remove stdtx.FeePayer()

b513a95

ValarDragon requested a review from zramsay as a code owner September 24, 2018 17:48

ValarDragon and others added 2 commits September 24, 2018 21:27

Merge branch 'develop' into dev/cleanup_auth

fb657a4

Fix lint

1c59807

jaekwon reviewed Sep 25, 2018

View reviewed changes

amend comment per Jae's suggestion

543cb7f

jaekwon reviewed Sep 25, 2018

View reviewed changes

make gasCost use int64 instead of float

344aab8

ValarDragon force-pushed the dev/cleanup_auth branch from 13f740f to 344aab8 Compare September 25, 2018 05:06

jaekwon reviewed Sep 25, 2018

View reviewed changes

rename variable

2d10de3

alexanderbez approved these changes Sep 26, 2018

View reviewed changes

ValarDragon mentioned this pull request Sep 26, 2018

switch adjustFeesForGas to use sdk.Coins #2411

Closed

cwgoes approved these changes Sep 26, 2018

View reviewed changes

cwgoes merged commit cb86efa into develop Sep 26, 2018

cwgoes deleted the dev/cleanup_auth branch September 26, 2018 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: Don't recalculate mempool fees for every msg signer, misc. cleanup #2376

auth: Don't recalculate mempool fees for every msg signer, misc. cleanup #2376

ValarDragon commented Sep 22, 2018 •

edited

Loading

codecov bot commented Sep 22, 2018 •

edited

Loading

ValarDragon Sep 22, 2018

ValarDragon commented Sep 22, 2018

ValarDragon Sep 22, 2018

ValarDragon Sep 22, 2018

ValarDragon Sep 22, 2018

ValarDragon Sep 22, 2018

ValarDragon Sep 22, 2018

alessio left a comment

alexanderbez left a comment

alexanderbez Sep 24, 2018

ValarDragon Sep 24, 2018

ValarDragon Sep 24, 2018 •

edited

Loading

alessio Sep 24, 2018

alexanderbez Sep 24, 2018 •

edited

Loading

alessio Sep 24, 2018

ValarDragon Sep 24, 2018

alexanderbez Sep 24, 2018

jaekwon Sep 25, 2018

jaekwon Sep 25, 2018

jaekwon Sep 25, 2018

ValarDragon commented Sep 26, 2018

alexanderbez commented Sep 26, 2018 •

edited

Loading

alexanderbez left a comment

cwgoes left a comment

auth: Don't recalculate mempool fees for every msg signer, misc. cleanup #2376

auth: Don't recalculate mempool fees for every msg signer, misc. cleanup #2376

Conversation

ValarDragon commented Sep 22, 2018 • edited Loading

codecov bot commented Sep 22, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ValarDragon commented Sep 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Sep 26, 2018

alexanderbez commented Sep 26, 2018 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

ValarDragon commented Sep 22, 2018 •

edited

Loading

codecov bot commented Sep 22, 2018 •

edited

Loading

ValarDragon Sep 24, 2018 •

edited

Loading

alexanderbez Sep 24, 2018 •

edited

Loading

alexanderbez commented Sep 26, 2018 •

edited

Loading