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

x/auth: Get accounts 1 by 1 #3093

Closed
ValarDragon opened this issue Dec 12, 2018 · 3 comments
Closed

x/auth: Get accounts 1 by 1 #3093

ValarDragon opened this issue Dec 12, 2018 · 3 comments
Assignees
Labels
T: Performance Performance improvements

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Dec 12, 2018

In auth we currently do the state read for all accounts, and then verify signatures one by one, stopping at the first failed signature. This is sub-optimal, as the first signature may be invalid but we've done the IO for all the other accounts. We should instead get the accounts one by one, and then verify its signature. This is important to do since this account checking logic can get spammed via CheckTx.

This should end up not being a honest-case significant performance hit as we don't gain much speed by batching those I/O operations. (IAVL isn't doing a batch read, and the relevant memory pages won't leave RAM under decent RAM assumptions. This will hurt instruction caching, but I think its worth it given that each of these I/O reads are independent and saving one I/O read far outweighs time lossed due to instruction cache misses. )

@ValarDragon ValarDragon added the T: Performance Performance improvements label Dec 12, 2018
@alexanderbez
Copy link
Contributor

This is the context of the ante handler I take it?

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 13, 2018

Yes

@alessio
Copy link
Contributor

alessio commented Dec 14, 2018

func getSignerAccs(ctx sdk.Context, am AccountKeeper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) {
	accs = make([]Account, len(addrs))
	for i := 0; i < len(accs); i++ {
		accs[i] = am.GetAccount(ctx, addrs[i])
		if accs[i] == nil {
			return nil, sdk.ErrUnknownAddress(addrs[i].String()).Result()
		}
	}

	return
}

@alessio alessio self-assigned this Dec 14, 2018
alessio pushed a commit that referenced this issue Dec 14, 2018
- Don't read all accounts in one go as signature verification
  could fail before last signature is checked.

- TestAnteHandlerFees now checks whether fees were actually
  deducted from the account as well as the fee keeper collected
  them.

Thanks: @ValarDragon for pointing this out
Closes: #3093
alessio pushed a commit that referenced this issue Dec 18, 2018
- Don't read all accounts in one go as signature verification
  could fail before last signature is checked.

- TestAnteHandlerFees now checks whether fees were actually
  deducted from the account as well as the fee keeper collected
  them.

Thanks: @ValarDragon for pointing this out
Closes: #3093
alessio pushed a commit that referenced this issue Dec 18, 2018
- Don't read all accounts in one go as signature verification
  could fail before last signature is checked.

- TestAnteHandlerFees now checks whether fees were actually
  deducted from the account as well as the fee keeper collected
  them.

Thanks: @ValarDragon for pointing this out
Closes: #3093
cwgoes pushed a commit that referenced this issue Dec 18, 2018
* x/auth: fetch one account after another

- Don't read all accounts in one go as signature verification
  could fail before last signature is checked.

- TestAnteHandlerFees now checks whether fees were actually
  deducted from the account as well as the fee keeper collected
  them.

Thanks: @ValarDragon for pointing this out
Closes: #3093
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Performance Performance improvements
Projects
None yet
Development

No branches or pull requests

3 participants