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.

Sign up for GitHub

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

Jump to bottom

Issue #210: Check required_auths on custom_operation #1629

Merged
merged 7 commits into from
Aug 13, 2019
Merged

Issue #210: Check required_auths on custom_operation #1629

merged 7 commits into from
Aug 13, 2019

Conversation

nathanielhourt
Copy link
Contributor

This PR resolves issue #210. See issue for discussion.

pmconrad
pmconrad suggested changes Mar 2, 2019
View reviewed changes
libraries/chain/include/graphene/chain/protocol/transaction.hpp Outdated Show resolved Hide resolved
libraries/chain/include/graphene/chain/protocol/transaction.hpp Outdated Show resolved Hide resolved
libraries/chain/include/graphene/chain/protocol/transaction.hpp Outdated Show resolved Hide resolved
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
libraries/chain/proposal_object.cpp Outdated Show resolved Hide resolved
libraries/chain/protocol/transaction.cpp Outdated Show resolved Hide resolved
tests/tests/authority_tests.cpp Show resolved Hide resolved
tests/tests/authority_tests.cpp Show resolved Hide resolved
libraries/chain/proposal_object.cpp Outdated Show resolved Hide resolved
@nathanielhourt
Copy link
Contributor Author

OK, did I get everything?

I did some refactoring in proposal_create_evaluator while I was in there, too. Hopefully it all looks good. The goal was to remove some redundant calculations, and move computation out of db.create() and into do_evaluate().

pmconrad
pmconrad previously requested changes Mar 4, 2019
View reviewed changes
libraries/app/database_api.cpp Outdated Show resolved Hide resolved
libraries/app/database_api.cpp Outdated Show resolved Hide resolved
libraries/chain/db_notify.cpp Outdated Show resolved Hide resolved
libraries/chain/hardfork.d/CORE_210.hf Show resolved Hide resolved
libraries/chain/proposal_evaluator.cpp Outdated Show resolved Hide resolved
libraries/chain/protocol/transaction.cpp Outdated Show resolved Hide resolved
@abitmore abitmore mentioned this pull request Mar 13, 2019
Open
34 tasks
@abitmore abitmore modified the milestones: 201810-Consensus-Release, 201810 - Consensus-Upgrade Release, 201911 - Consensus-Upgrade Release Mar 13, 2019
@nathanielhourt nathanielhourt dismissed pmconrad’s stale review June 25, 2019 12:22

Changes already applied, no?

@pmconrad
Copy link
Contributor

Please rebase on latest hardfork branch.

nathanielhourt and others added 5 commits July 30, 2019 22:42
@nathanielhourt
Resolve bitshares#210: [HF] Check authorities on custom_operation
9d04ddb 
The required_auths field on custom_operation was being ignored during authority checking. This commit causes it to be checked correctly, and adds a unit test verifying as much.
@nathanielhourt
Ref bitshares#210: Add hardfork guards
771e038 
Hardfork guards are complex for this issue, because we can't access
chain time in the operation's get_required_active_authorities() method.

There was no apparent 'good' way to solve this, so I settled for a
fairly nasty hack which seemed the least bad of bad options. Add a
boolean parameter to the verify_authority() functions determining
whether the custom_operation::required_auths field should be ignored or
not, and call these functions setting the boolean appropriately for
whether we have passed the hardfork or not. This works because chain
time is available at the verify_authority() call sites.
@nathanielhourt
Progress bitshares#210: Apply requested changes
974c8b6
@nathanielhourt
Progress bitshares#210: Requested changes
d5503e2 
Mostly polish, but one mistake fix as well.
@nathanielhourt
Ref bitshares#210: More explicit lambda captures
a081fff
@nathanielhourt
Copy link
Contributor Author

OK, this should be ready. Let me know if I missed anything.

pmconrad
pmconrad suggested changes Aug 1, 2019
View reviewed changes
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Looks good, but one more thing:

Docker build is consistently failing in db_notify.cpp because it it doesn't know the MUST_IGNORE_CUSTOM_OP_REQD_AUTHS macro. I think db_notify.cpp must include hardfork.hpp (db_block.cpp does include it and is compiled successfully).

The difference between Travis and Docker is that Docker uses -DGRAPHENE_DISABLE_UNITY_BUILD=ON but Travis doesn't. I. e. Travis compiles all db_*.cpp in one go which means that the include in db_block.cpp also works for db_notify.cpp. Docker compiles db_notify.cpp separately, so the include is missing.

@nathanielhourt nathanielhourt merged commit ab54bcb into bitshares:hardfork Aug 13, 2019
@nathanielhourt nathanielhourt deleted the issue-210-hardfork branch August 13, 2019 18:11
@MichelSantos MichelSantos mentioned this pull request Oct 18, 2019
Closed
@abitmore abitmore mentioned this pull request Mar 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abitmore abitmore abitmore left review comments

@pmconrad pmconrad pmconrad approved these changes

Assignees
No one assigned
Labels
hardfork
Projects
None yet
Milestone
4.0.0 - Protocol Upgrade Release
Development

Successfully merging this pull request may close these issues.
4 participants
@nathanielhourt @pmconrad @abitmore @oxarbitrage