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

[1.0-beta1.1] Store proposed producers and proposed finalizers in building block #189

Merged
merged 2 commits into from
May 23, 2024

Conversation

heifner
Copy link
Member

@heifner heifner commented May 22, 2024

The chainbase global_property_object was used to store finalizer_policy during block construction as a means to provide automatic rollback if the transaction/block failed. See #99. However, finalizer_policy contains non-shared types of std::vector and fc::crypto::blslib::bls_public_key.

This PR solves the problem of avoiding side-effects of proposed finalizer policy made in an aborted transaction in a different way than committing the speculative changes into the global_property_object in chainbase. Now we store the speculative changes in the transaction context. Then if we commit the transaction, it copies them over (assuming they are not nullopt) from the transaction context to the building block. This avoids having to add a shared version of the finalizer_policy and bls_public_key.

For symmetry, the proposer schedule in Savanna was modified to also be managed the same way as finalier_policy in transaction_context.

This PR removes proposed_fin_pol_block_num and proposed_fin_pol from global_property_object and therefore is not state compatible with previous versions. The proposed schedule proposed_schedule_block_num & proposed_schedule were not removed from global_property_object because they are still needed pre-Savanna.

Note PR #188 targets main. main includes producer and finalizer diffs. This is a cherry-pick of #188 with additional changes to fix merge conflicts.

Resolves #182

heifner added 2 commits May 22, 2024 14:13
…lock instead of global_property_object. Fixes bug with non-shared types of finalizer_policy being stored in chainbase.
@heifner heifner requested review from linh2931 and spoonincode May 22, 2024 19:24
@heifner heifner added the OCI Work exclusive to OCI team label May 22, 2024
@heifner heifner linked an issue May 22, 2024 that may be closed by this pull request
spoonincode
spoonincode previously approved these changes May 23, 2024
@spoonincode spoonincode dismissed their stale review May 23, 2024 15:49

one sec

// proposed_schedule.version is set in assemble_block
trx_blk_context.proposed_schedule.producers = std::move(producers);

return std::numeric_limits<uint32_t>::max();
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be a uint32_t instead of int64_t? I realize that's how it was before this PR but the types being mismatched here looks sus

Copy link
Member Author

@heifner heifner May 23, 2024

Choose a reason for hiding this comment

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

Yes, that is in accordance to what @arhag requested.

* @return pre-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
* post-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns max uint32_t

@heifner heifner merged commit b4d97aa into release/1.0-beta1 May 23, 2024
36 checks passed
@heifner heifner deleted the GH-182-gpo-beta1 branch May 23, 2024 16:20
@ericpassmore
Copy link
Contributor

Note:start
group: IF
category: BUG
summary: Improve data safety for transaction rollback and avoid unintended side effects for a proposed finalizer policy change during an aborted transaction.
Note:end

@ericpassmore
Copy link
Contributor

Validated this is working

info  2024-05-23T19:46:59.964 nodeos    controller.cpp:1363           transition_to_savann ] Transitioning to savanna, IF Genesis Block 641, IF Critical Block 686
info  2024-05-23T19:46:59.964 nodeos    controller.cpp:1384           operator()           ] Transition to instant finality happening after block 686, First IF Proper Block 687
info  2024-05-23T19:52:24.899 nodeos    block_header_state.cpp:187    finish_next          ] New finalizer policy becoming active in block 0000052ce7505b324970d137ae0aff7a717ba812eb8c6000abecb0bbc593aea6: {"generation":2,"threshold":3,"finalizers":[{"description":"bpa","weight":1,"public_key":"PUB_BLS_n9zAW7r9sx6FvjDb_9e64ozf4MPXdO62LWkxtjZWkK-e5K_jvpJa0ugv8sy87UoUzt4I_FldyNnmFCv7wBdIAEAQHbHeiYz-izo2iaA9XAAufT4375SClQ8XUJAScjsRX9pUgw"},{"description":"bpc","weight":1,"public_key":"PUB_BLS_P38d0wxZuM668D6BHCdBOw5jjlhuVXqa6i7t7Hfa6CAY4GcoM6u8ruIYs7oKulMKz2cnJtEyuABM564mbRpXMvUd_stxJoOSyt_FHzea4I07vnMPrIlSPX1pYXffS-AS5tfITg"},{"description":"bpb","weight":1,"public_key":"PUB_BLS_RoazjGvyIFc9Jc7JSB7TTIJAKgPj43cabIgJ5PHj2zQP0pMfyzzQIR_JaMNTJ6YWaPNBqbUCgxtFoF-dd0Yjfxpp49faH-8ZgjcSF86K4AqMopl4k0juxE3x0QJCwVcDuVbrbw"}]}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodeos Crashes after actfinkey action
4 participants