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

Authorities on custom_operation are not checked correctly #210

Closed
vikramrajkumar opened this issue Jan 18, 2017 · 7 comments
Closed

Authorities on custom_operation are not checked correctly #210

vikramrajkumar opened this issue Jan 18, 2017 · 7 comments
Assignees
@nathanielhourt @oxarbitrage
Labels
bug hardfork
Milestone
4.0.0 - Protocol ...

Comments

@vikramrajkumar
Copy link
Contributor

From @nathanhourt on May 9, 2016 22:6

The required_auths field on custom_operation should require extra signatures for the accounts listed, but it does not:
https://bitshares.openledger.info/#/block/5975061

Copied from original issue: cryptonomex/graphene#649
@vikramrajkumar vikramrajkumar mentioned this issue Jan 18, 2017
Closed
@abitmore
Copy link
Member

abitmore commented May 9, 2017 •
edited
Loading

why required_auths is flat_set<account_id_type> but not flat_set<authority>? I think the latter makes more sense (edit: so we can also specify keys).

@oxarbitrage oxarbitrage modified the milestones: Roadmap to hardfork, Hardfork - Operations related, Roadmap to hardfork 1 Aug 13, 2017
@abitmore
Copy link
Member

There is a fix: FollowMyVote/graphene@0bb157e

@abitmore abitmore added the bug label Nov 28, 2017
@abitmore abitmore modified the milestones: Hardfork - Operations and Authority related., Future Consensus-Changing Release Nov 28, 2017
@nathanielhourt
Copy link
Contributor

Gack! This is difficult to write hardfork guards for... You don't have access to chain time in op::get_required_active_authorities()...

I'll see if I can pass that in as another parameter.

@pmconrad
Copy link
Contributor

pmconrad commented Mar 1, 2019 •
edited
Loading

I've solved a similar problem in MUSE aka SOUNDAC by introducing a version parameter in transaction::verify_authority and modifying validation logic depending on that. The correct version to apply is determined externally (i. e. in database and in API).
https://github.com/soundac/SounDAC-Source/blob/0.3.2/libraries/chain/protocol/transaction.cpp#L410

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Resolve bitshares#210: [HF] Check authorities on custom_operation
95427b0 
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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Ref bitshares#210: Add hardfork guards
b2529d6 
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
Copy link
Contributor

nathanielhourt commented Mar 1, 2019 •
edited
Loading

@pmconrad I didn't see your comment until now, but it seems that we both settled on the same solution. :) I'll take that to mean it's a reasonable solution.

I believe this issue to be resolved; am I missing anything before I PReq? My patch is here.

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Resolve bitshares#210: [HF] Check authorities on custom_operation
9c78b53 
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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Ref bitshares#210: Add hardfork guards
ff49d20 
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.
@pmconrad
Copy link
Contributor

pmconrad commented Mar 1, 2019

Looks good at first glance. Please create PR to hardfork branch.

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Resolve bitshares#210: [HF] Check authorities on custom_operation
fab33f3 
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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Ref bitshares#210: Add hardfork guards
53ab416 
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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 1, 2019
@nathanielhourt
Ref bitshares#210: Add hardfork guards
33a221f 
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 nathanielhourt mentioned this issue Mar 1, 2019
Merged
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 4, 2019
@nathanielhourt
Progress bitshares#210: Apply requested changes
e8ef81b
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 6, 2019
@nathanielhourt
Progress bitshares#210: Requested changes
466ed7c 
Mostly polish, but one mistake fix as well.
@abitmore abitmore modified the milestones: Future Consensus-Upgrade Release, 201810-Consensus-Release Mar 13, 2019
@abitmore abitmore modified the milestones: 201810 - Consensus-Upgrade Release, 201911 - Consensus-Upgrade Release Mar 13, 2019
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Mar 13, 2019
@nathanielhourt
Ref bitshares#210: More explicit lambda captures
06e9090
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@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 added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@nathanielhourt
Progress bitshares#210: Apply requested changes
974c8b6
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@nathanielhourt
Progress bitshares#210: Requested changes
d5503e2 
Mostly polish, but one mistake fix as well.
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@nathanielhourt
Ref bitshares#210: More explicit lambda captures
a081fff
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Jul 31, 2019
@nathanielhourt
Issue bitshares#210: Formatting and build fixes
48a5457
nathanielhourt added a commit that referenced this issue Aug 13, 2019
@nathanielhourt
Merge pull request #1629 from nathanhourt/issue-210-hardfork
ab54bcb 
Issue #210: Check required_auths on custom_operation
@nathanielhourt
Copy link
Contributor

This is merged to hardfork

jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Resolve #210: [HF] Check authorities on custom_operation
8675549 
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.
jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Ref #210: Add hardfork guards
178d1be 
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.
jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Progress #210: Apply requested changes
0b9cd9b
jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Progress #210: Requested changes
a24345e 
Mostly polish, but one mistake fix as well.
jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Ref #210: More explicit lambda captures
da5e985
jmjatlanta pushed a commit that referenced this issue Sep 3, 2019
@nathanielhourt @jmjatlanta
Issue #210: Formatting and build fixes
54bb60c
@MichelSantos MichelSantos mentioned this issue Oct 18, 2019
Closed
@abitmore abitmore mentioned this issue Mar 25, 2020
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nathanielhourt nathanielhourt

@oxarbitrage oxarbitrage

Labels
bug hardfork
Projects
None yet
Milestone
4.0.0 - Protocol Upgrade Release
Development

No branches or pull requests
5 participants
@nathanielhourt @vikramrajkumar @pmconrad @abitmore @oxarbitrage