Peerplays_Blockchain/peerplays_migrated
1
0
Fork 0
Code Issues 35 Pull requests 4 Projects Releases 20 Packages Wiki Activity Actions

Change default constructor of num_son extension in account_options #789

Closed
milo_peerplays wants to merge 4 commits from bug/513-num_son_extension into develop
pull from: bug/513-num_son_extension
merge into: Peerplays_Blockchain:develop
Conversation 18 Commits 4 Files changed 2 +24 -9
milo_peerplays commented 2023-02-19 15:00:35 +00:00 (Migrated from gitlab.com)
Copy link
No description provided.
serkixenos commented 2023-02-21 05:05:59 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in issue #513

mentioned in issue #513
milo_peerplays commented 2023-02-23 00:13:48 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 8297ff5b - fix tests

Compare with previous version

added 1 commit <ul><li>8297ff5b - fix tests</li></ul> [Compare with previous version](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=611094759&start_sha=f835673b8cf8a76842f8ceba207f4f264db081ef)
milo_peerplays commented 2023-02-23 12:26:14 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 6145c3c8 - Change default constructor of account_options

Compare with previous version

added 1 commit <ul><li>6145c3c8 - Change default constructor of account_options</li></ul> [Compare with previous version](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=611606077&start_sha=8297ff5b8119868b7632a42e7b7bab6f06f0acbf)
christophersanborn commented 2023-02-23 17:36:55 +00:00 (Migrated from gitlab.com)
Copy link

IMHO this code addition is unnecessary, (although it is better than the code it replaces). If the user has not elected to set the num_son parameter upon account creation, then better that the account object that the node retains reflects this accurately, rather than try to sub-in a default zero-initialized num_son array.

There are two reasons for this:

(1) — There is (to my knowledge) no "data consistency" requirement that the value be initialized. (Or at least, there oughtn't be.) — Accounts that exist on-chain before the num_son hard-fork takes effect will not have this value set. Therefore, new accounts shouldn't need it either. Similarly, in the future when new side-chain types are added, the array initialized for this account will become "too short" anyway. Better would be if, in the place where this value is needed, we ensure it cleanly handles array elements that are missing/unset. (Including the case when the entire array is un-set.)

(2) — It is a maintenance burden. The account_create operation is not the only operation where the account_options object is used. It is also used in the account_update operation. For completeness, if we keep to the initialization strategy here, we would need to replicate it also in the do_apply() for account_update. There are then two areas of code that may need future attention for changes.

Suggestion: If num_son is unset, leave it unset. BUT — please verify that in the area of code where we compute the active num_son numbers, that it is not strictly expecting a well-formed array. That area of code must be sophisticate enough to accept an unset value (or, in the future when new side-chains are added, an array that is too short) and treat it as if the unset array elements are zero. This then should be robust against accounts that do not specify a full list of num_son values.

IMHO this code addition is unnecessary, (although it is better than the code it replaces). If the user has not elected to set the `num_son` parameter upon account creation, then better that the account object that the node retains reflects this accurately, rather than try to sub-in a default zero-initialized num_son array. There are two reasons for this: (1) — There is (to my knowledge) no "data consistency" requirement that the value be initialized. (Or at least, there oughtn't be.) — Accounts that exist on-chain before the num_son hard-fork takes effect will not have this value set. Therefore, new accounts shouldn't need it either. Similarly, in the future when new side-chain types are added, the array initialized for this account will become "too short" anyway. Better would be if, in the place where this value is needed, we ensure it cleanly handles array elements that are missing/unset. (Including the case when the entire array is un-set.) (2) — It is a maintenance burden. The account_create operation is not the only operation where the account_options object is used. It is also used in the account_update operation. For completeness, if we keep to the initialization strategy here, we would need to replicate it also in the `do_apply()` for account_update. There are then two areas of code that may need future attention for changes. Suggestion: If `num_son` is unset, leave it unset. BUT — please verify that in the area of code where we compute the active num_son numbers, that it is not strictly expecting a well-formed array. That area of code must be sophisticate enough to accept an unset value (or, in the future when new side-chains are added, an array that is too short) and treat it as if the unset array elements are zero. This then should be robust against accounts that do not specify a full list of num_son values.
christophersanborn commented 2023-02-23 17:36:55 +00:00 (Migrated from gitlab.com)
Copy link

If reverting the addition below, please do not restore this line. In fact, better to remove the entire conditional (I don't believe it is needed), or to modify this line to explicitly set only the num_son parameter. As written the line replaces the entire extensions element, which, although at present includes only the num_son parameter,... in the future this may clobber additional added parameters not yet included in the account_options::ext class.

If reverting the addition below, please do not restore this line. In fact, better to remove the entire conditional (I don't believe it is needed), or to modify this line to explicitly set only the num_son parameter. As written the line replaces the entire extensions element, which, although at present includes only the num_son parameter,... in the future this may clobber additional added parameters not yet included in the `account_options::ext` class.
milo_peerplays commented 2023-02-23 22:27:22 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=612213219&start_sha=6145c3c89b4b758a10d5564c6973cfdcaff4e8d0#74a921df41380fb5dcc5dde9ab2be20669888835_196_196)
milo_peerplays commented 2023-02-23 22:27:22 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>188d5515 - Fix tests</li></ul> [Compare with previous version](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=612213219&start_sha=6145c3c89b4b758a10d5564c6973cfdcaff4e8d0)
milo_peerplays commented 2023-02-24 14:27:02 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 5e7acde8 - Also cover account_update_operation

Compare with previous version

added 1 commit <ul><li>5e7acde8 - Also cover account_update_operation</li></ul> [Compare with previous version](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=612873956&start_sha=188d55158c1ccd62313abdc0b90f590150d7ca4a)
christophersanborn commented 2023-02-24 15:41:33 +00:00 (Migrated from gitlab.com)
Copy link

Update: Although initializing here is not strictly ideal, it is workable so longs as all operations that update account_options objects have a similar initializer. The ideal solution can be future work on a separate ticket, so, marking this specific comment thread as resolved.

Update: Although initializing here is not strictly ideal, it is workable so longs as all operations that update account_options objects have a similar initializer. The ideal solution can be future work on a separate ticket, so, marking this specific comment thread as resolved.
christophersanborn commented 2023-02-24 15:52:18 +00:00 (Migrated from gitlab.com)
Copy link

The line:

a.options.extensions.value = account_options::ext();

will reset all elements of the extensions struct, not just num_son, and thus may have unintended side-effects (specifically in the future when/if additional extensions are added). This line can and should be removed.

The line: ```cpp a.options.extensions.value = account_options::ext(); ``` will reset all elements of the extensions struct, not just `num_son`, and thus may have unintended side-effects (specifically in the future when/if additional extensions are added). This line can and should be removed.
christophersanborn commented 2023-02-24 15:52:18 +00:00 (Migrated from gitlab.com)
Copy link

Similar to comment on line 349 — This line will reset all elements of the extensions struct, not just num_son, and thus may have unintended side-effects (specifically in the future when/if additional extensions are added). This line can and should be removed.

Similar to comment on line 349 — This line will reset all elements of the extensions struct, not just `num_son`, and thus may have unintended side-effects (specifically in the future when/if additional extensions are added). This line can and should be removed.
christophersanborn commented 2023-02-24 15:52:18 +00:00 (Migrated from gitlab.com)
Copy link

Aside from the two one-line edits suggested, the proposed solution here looks workable to me. Should be good to merge following suggested edits.

Aside from the two one-line edits suggested, the proposed solution here looks workable to me. Should be good to merge following suggested edits.
milo_peerplays commented 2023-02-25 11:06:20 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=613357659&start_sha=5e7acde8851ad5d1abd69d49fbed4b962025e1dc#74a921df41380fb5dcc5dde9ab2be20669888835_349_348)
milo_peerplays commented 2023-02-25 11:06:21 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=613357659&start_sha=5e7acde8851ad5d1abd69d49fbed4b962025e1dc#74a921df41380fb5dcc5dde9ab2be20669888835_196_196)
milo_peerplays commented 2023-02-25 11:06:21 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 3f558da2 - Do not overwrite value of account_options extension

Compare with previous version

added 1 commit <ul><li>3f558da2 - Do not overwrite value of account_options extension</li></ul> [Compare with previous version](/PBSA/peerplays/-/merge_requests/209/diffs?diff_id=613357659&start_sha=5e7acde8851ad5d1abd69d49fbed4b962025e1dc)
christophersanborn commented 2023-02-26 16:51:38 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in issue PBSA/tools-libs/python-peerplays#170

mentioned in issue PBSA/tools-libs/python-peerplays#170
christophersanborn commented 2023-02-26 16:56:39 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in issue PBSA/tools-libs/faucet#31

mentioned in issue PBSA/tools-libs/faucet#31
vampik (Migrated from gitlab.com) closed this pull request 2023-03-03 07:42:13 +00:00
vampik commented 2023-03-03 07:42:48 +00:00 (Migrated from gitlab.com)
Copy link

New implementation is here:
https://gitlab.com/PBSA/peerplays/-/merge_requests/213

New implementation is here: https://gitlab.com/PBSA/peerplays/-/merge_requests/213

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
confirmed

   
DevOps

   
documentation

   
enhancement

   
incident

Denotes a disruption to IT services and the associated issues require immediate attention

  
Medium

   
priority::Critical

(optional) Issues that are marked as critical priority. Should be taken care of ASAP.

  
priority::High

(optional) Issues that are marked as high priority.

  
priority::Low

(optional) Issues that are marked as low priority.

  
priority::Medium

(optional) Issues that are marked as medium priority.

  
QA-TestCases::Done

QA-Labels

  
QA-TestCases::InProgress

QA-Labels

  
RNG

   
security

(optional) Issues that relate to the security of the project.

  
severity:: Critical

   
severity:: High

   
severity:: Low

   
severity:: Medium

   
state:: Accepted

Issues that are newly created and have been accepted after review.

  
state:: Blocked

Issues that cannot be worked on due to other factors linked to it.

  
state:: Done

Issues that are closed with a working solution.

  
state:: in progress

Issues that are currently being worked on.

  
state:: in review

Issues that require a proposed solution to be reviewed.

  
state:: on hold

Issues that were at least accepted, that should not be worked on at the moment.

  
state:: Pending

Issues that are newly created and haven't been reviewed.

  
state:: Pushed to Prod

Tickets are verified in the stage infra and needs to be deployed in the production

  
state:: Revision Needed

Issues that need to be revised before being accepted.

  
state::out-of-scope

Task and issues which are out of scope due to change in requirements

  
suggestion

   
support

   
To Do

   
type :: Discussion

   
type :: Duplicate

(optional) Issues that are duplicates of existing issues in the same project.

  
type:: interaction-design

   
type:: New Feature

Issues that bring new functionality to the project.

  
type::bug

BUG

  
type::documentation

Issues that relate to the documentation of the project.

  
type::incident

severe issues that require immediate response

  
type::maintenance

Issues that require maintenance on the project.

  
type::question

Issues that are questions related to the project.

  
type::recurring-tasks

This label is used to track recurring tasks which may repeat daily, weekly or monthly.

  
vulnerability
No labels
confirmed
DevOps
documentation
enhancement
incident
Medium
priority::Critical
priority::High
priority::Low
priority::Medium
QA-TestCases::Done
QA-TestCases::InProgress
RNG
security
severity:: Critical
severity:: High
severity:: Low
severity:: Medium
state:: Accepted
state:: Blocked
state:: Done
state:: in progress
state:: in review
state:: on hold
state:: Pending
state:: Pushed to Prod
state:: Revision Needed
state::out-of-scope
suggestion
support
To Do
type :: Discussion
type :: Duplicate
type:: interaction-design
type:: New Feature
type::bug
type::documentation
type::incident
type::maintenance
type::question
type::recurring-tasks
vulnerability
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Peerplays_Blockchain/peerplays_migrated#789
No description provided.
Delete branch "bug/513-num_son_extension"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?