Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Progress on #7, upgrades to testing framework #151

Merged
merged 7 commits into from
Aug 10, 2017

Conversation

nathanielhourt
Copy link
Contributor

We now check that transactions don't have any unnecessary signatures.

Upgrade AuthorityChecker to support tracking which keys were used. This makes it possible to check for irrelevant sigs, and also makes it possible to automatically sign transactions by taking a list of available keys and determining which ones are necessary to authorize the transaction.

Upgrade testing framework to support automatically signing transactions (not yet working as of 7eb2013)

Allow scoped_exit objects to be canceled so they do not call their
callbacks when destroyed.
Move logic from apply_context::unused_authorizations out to a header so
other code can use it.

The logic in question takes an array of data, and a parallel array of
"markers" (booleans, in this instance), and returns a range containing
only the data elements whose corresponding marker matches a provided
value (i.e. the boolean was true).

Stay tuned to see how else I use this code! ;)
I want two things from AuthorityChecker that it wasn't doing yet:
1. I want it to track which of the provided keys were used, so I can
reject transactions which bear more signatures than are necessary
2. To sign a transaction with no unnecessary keys, I want it to support
taking a list of available public keys and an authority, then telling me
which of those keys I should use to fully satisfy the authority, without
having any unnecessary keys

As an added bonus, having both of these operations handled by
AuthorityChecker means that determining the set of keys needed to sign a
transaction, and determining whether a transaction is properly signed,
use the same code. :D
Implement automatic signing of transactions in the test framework.
Currently disabled, as it's not yet working
Previously, validate_referenced_accounts was called during transaction
application; however, account creation is one of those changes that
doesn't really take effect until the end of the block, so we want to
validate the referenced accounts for all transactions at the beginning
of the block so that if trx A creates account joe, then trx B references
joe, that reference will fail if A and B are in the same block. We also
call it during push_transaction, as we do want to check it for pending
transactions that aren't in a block yet.
Copy link
Contributor

@wanderingbort wanderingbort left a comment

Choose a reason for hiding this comment

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

Just a few questions before approval. Otherwise, it looks good.

vector<public_key_type> signingKeys;
vector<bool> usedKeys;

struct WeightTallyVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style note: I lean towards stateless visitors when I can in order to improve the colocation of code and intent. Below in AuthorityChecker::satisfied we loop over all the permissions mutating the internal state of a visitor. The only hint that this is happening in the context of that method's scope is the use of Tally in the visitor's type.

I am not going to ding a review for it but, I would consider making the visitor simply produce the weight and then putting the aggregation of weights in the ::satisfied method where the result is important/used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, on second blush there are a few hidden side effects in this visitor, like usedKeys mutation. So, making it "pure" from an aggregation point of view is rather hollow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the whole AuthorityChecker class got pretty complex, but what it's doing is also pretty bloody complex... I didn't come up with a simpler approach, but perhaps one exists. I'm open to suggestions. In the meantime, I designed it to hide all the details inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider making the visitor simply produce the weight and then putting the aggregation of weights in the ::satisfied method where the result is important/used.

Not sure I understand what you mean here. Do you mean not taking the early out you mentioned below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, you mean add up the weights in satisfied... Seems like 6 of one, half dozen of the other to me, but sure, I can change it.

// If we've got enough weight, to satisfy the authority, return!
if (permission.visit(visitor) >= authority.threshold) {
KeyReverter.cancel();
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we concerned that this early out may lead to problems when permissions are over-authorized?
For instance, if a user were to have 2 messages that required (2 of 3: A, B, C) and (1 of 3: A, B) and had signatures from A, B and C. Depending on the order of the checks this will either throw for unecessary sigs (A,B pass, A pass) or not (B, C pass, A pass).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see what affect breaking early has on that... but we definitely want to break early, some of these authorities can be quite expensive to check (many recursions down an account multisig tree, for instance) and we don't want to do that if we already know the answer.

As to the case where two messages might let the user slip some extra sigs in... Can they, though? Authorities should be sorted, so we should always check the keys within them in the same order... That should prevent the situation you described, yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

the sorting certainly means that for any transaction this always goes the same way. My concern is the burden on wallets to match this algorithm in their signature collection and pruning. There should be a usable set of rules for this pruning but it seems excessive.

A wallet must prune any signatures made irrelevant by this evaluation algorithm or it risks being rejected. Likely this means it will collect signatures and re-run this algorithm with the early out to determine which of the obtained signatures will be marked as "unused" in the case of over authorization.

I understand that over-authorization has costs but the rules for a properly formed transaction are made more complex by this early out. Without it, we may exceed the authority threshold but we will mark all signatures that could be part of a message authorization as used. Relieving the burden of wallets but putting potentially more stress on the chain/processing.

Copy link
Contributor Author

@nathanielhourt nathanielhourt Aug 10, 2017

Choose a reason for hiding this comment

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

I guess the ordering requirement only means that we have consistent behavior across messages, not that we would detect that oversigning issue you described. I suppose to address that, I can make WeightTallyVisitor check if keys that are already used would satisfy the authority first, and only then check unused keys/marking them used if they help.

I'll write up a test case to exercise this issue, then fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make the over-authorization a "soft" consensus rather than a "hard" consensus. In other words, block producers may choose to include over-authorized transactions, but in practice will reject them. This will allow us to update / refine this without hard forking the chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then again, I can't figure out a way to make WeightTallyVisitor mark a key as used when it didn't need to be used. Since it always checks keys in the same order, it always prefers using already-used keys to unused ones without me explicitly checking for it.

}
}
for (const auto& apw : authority.accounts)
//#warning TODO: Recursion limit? Yes: implement as producer-configurable parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

From now on all such TODO's need to reference a specific github issue number and should be tagged as "bug" and assigned to a final release milestone (June 2018) or earlier. We should also not flood our builds with warnings like this, especially in header files.

@bytemaster bytemaster merged commit c405fc9 into master Aug 10, 2017
ljrprocc pushed a commit to bithacks-tech/myeosio that referenced this pull request Jul 4, 2018
NorseGaud pushed a commit that referenced this pull request Jul 30, 2019
# This is the 1st commit message:

various improvements

# This is the commit message #2:

new hash

# This is the commit message #3:

fix for script path

# This is the commit message #4:

fixes

# This is the commit message #5:

fixes

# This is the commit message #6:

fixes

# This is the commit message #7:

fixes

# This is the commit message #8:

fixes

# This is the commit message #9:

fixes

# This is the commit message #10:

fixes

# This is the commit message #11:

fixes

# This is the commit message #12:

fixes

# This is the commit message #13:

fixes

# This is the commit message #14:

fixes

# This is the commit message #15:

fixes

# This is the commit message #16:

fixes

# This is the commit message #17:

fixes

# This is the commit message #18:

fixes

# This is the commit message #19:

fixes

# This is the commit message #20:

fixes

# This is the commit message #21:

fixes

# This is the commit message #22:

fixes

# This is the commit message #23:

fixes

# This is the commit message #24:

fixes

# This is the commit message #25:

fixes

# This is the commit message #26:

testing

# This is the commit message #27:

testing

# This is the commit message #28:

testing

# This is the commit message #29:

testing

# This is the commit message #30:

testing

# This is the commit message #31:

testing

# This is the commit message #32:

testing

# This is the commit message #33:

testing

# This is the commit message #34:

testing

# This is the commit message #35:

testing

# This is the commit message #36:

testing

# This is the commit message #37:

testing

# This is the commit message #38:

testing

# This is the commit message #39:

testing

# This is the commit message #40:

testing

# This is the commit message #41:

testing

# This is the commit message #42:

testing

# This is the commit message #43:

testing

# This is the commit message #44:

fixes

# This is the commit message #45:

fixes

# This is the commit message #46:

fixes

# This is the commit message #47:

fixes

# This is the commit message #48:

fixes

# This is the commit message #49:

fixes

# This is the commit message #50:

fixes

# This is the commit message #51:

fixes

# This is the commit message #52:

fixes

# This is the commit message #53:

fixes

# This is the commit message #54:

fixes

# This is the commit message #55:

fixes

# This is the commit message #56:

fixes

# This is the commit message #57:

fixes

# This is the commit message #58:

fixes

# This is the commit message #59:

fixes

# This is the commit message #60:

fixes

# This is the commit message #61:

fixes

# This is the commit message #62:

fixes

# This is the commit message #63:

fixes

# This is the commit message #64:

fixes

# This is the commit message #65:

fixes

# This is the commit message #66:

fixes

# This is the commit message #67:

fixes

# This is the commit message #68:

fixes

# This is the commit message #69:

fixes

# This is the commit message #70:

fixes

# This is the commit message #71:

fixes

# This is the commit message #72:

fixes

# This is the commit message #73:

fixes

# This is the commit message #74:

fixes

# This is the commit message #75:

fixes

# This is the commit message #76:

fixes

# This is the commit message #77:

fixes

# This is the commit message #78:

fixes

# This is the commit message #79:

more testing

# This is the commit message #80:

testing

# This is the commit message #81:

fixes

# This is the commit message #82:

fixes

# This is the commit message #83:

fixes

# This is the commit message #84:

fixes

# This is the commit message #85:

fixes

# This is the commit message #86:

fixes

# This is the commit message #87:

fixes

# This is the commit message #88:

fixes

# This is the commit message #89:

fixes

# This is the commit message #90:

fixes

# This is the commit message #91:

fixes

# This is the commit message #92:

fixes

# This is the commit message #93:

propagate-environment for buildkite-agent

# This is the commit message #94:

propagate-environment for buildkite-agent

# This is the commit message #95:

propagate-environment for buildkite-agent

# This is the commit message #96:

propagate-environment for buildkite-agent

# This is the commit message #97:

fixes

# This is the commit message #98:

fixes

# This is the commit message #99:

fixes

# This is the commit message #100:

fixes

# This is the commit message #101:

fixes

# This is the commit message #102:

fixes

# This is the commit message #103:

fixes

# This is the commit message #104:

fixes

# This is the commit message #105:

fixes

# This is the commit message #106:

fixes

# This is the commit message #107:

fixes

# This is the commit message #108:

fixes

# This is the commit message #109:

fixes

# This is the commit message #110:

fixes

# This is the commit message #111:

fixes

# This is the commit message #112:

fixes

# This is the commit message #113:

fixes

# This is the commit message #114:

fixes

# This is the commit message #115:

fixes

# This is the commit message #116:

fixes

# This is the commit message #117:

fixes

# This is the commit message #118:

fixes

# This is the commit message #119:

fixes

# This is the commit message #120:

fixes

# This is the commit message #121:

fixes

# This is the commit message #122:

fixes

# This is the commit message #123:

fixes

# This is the commit message #124:

fixes

# This is the commit message #125:

fixes

# This is the commit message #126:

fixes

# This is the commit message #127:

fixes

# This is the commit message #128:

fixes

# This is the commit message #129:

fixes

# This is the commit message #130:

fixes

# This is the commit message #131:

fixes

# This is the commit message #132:

fixes

# This is the commit message #133:

fixes

# This is the commit message #134:

fixes

# This is the commit message #135:

fixes

# This is the commit message #136:

fixes

# This is the commit message #137:

fixes

# This is the commit message #138:

fixes

# This is the commit message #139:

fixes

# This is the commit message #140:

fixes

# This is the commit message #141:

fixes

# This is the commit message #142:

fixes

# This is the commit message #143:

fixes

# This is the commit message #144:

fixes

# This is the commit message #145:

fixes

# This is the commit message #146:

fixes

# This is the commit message #147:

fixes

# This is the commit message #148:

fixes

# This is the commit message #149:

fixes

# This is the commit message #150:

fixes

# This is the commit message #151:

fixes

# This is the commit message #152:

fixes

# This is the commit message #153:

testing

# This is the commit message #154:

fixes

# This is the commit message #155:

fixes

# This is the commit message #156:

fixes

# This is the commit message #157:

fixes

# This is the commit message #158:

fixes

# This is the commit message #159:

fixes

# This is the commit message #160:

fixes

# This is the commit message #161:

fixes

# This is the commit message #162:

fixes

# This is the commit message #163:

fixes

# This is the commit message #164:

fixes

# This is the commit message #165:

fixes

# This is the commit message #166:

fixes

# This is the commit message #167:

fixes

# This is the commit message #168:

fixes

# This is the commit message #169:

fixes

# This is the commit message #170:

fixes

# This is the commit message #171:

fixes

# This is the commit message #172:

fixes

# This is the commit message #173:

fixes

# This is the commit message #174:

fixes

# This is the commit message #175:

fixes

# This is the commit message #176:

fixes

# This is the commit message #177:

fixes

# This is the commit message #178:

fixes

# This is the commit message #179:

fixes

# This is the commit message #180:

fixes

# This is the commit message #181:

fixes

# This is the commit message #182:

fixes

# This is the commit message #183:

fixes

# This is the commit message #184:

fixes

# This is the commit message #185:

fixes

# This is the commit message #186:

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

Successfully merging this pull request may close these issues.

3 participants