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.1] Should never move from a signed_block in a shared_ptr #738

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

heifner
Copy link
Member

@heifner heifner commented Sep 10, 2024

Should never move a signed_block that could still be used. The point of shared_ptr is shared data, you don't know if being used by something else unless you just created it. For example if still in the forkdb. Would have preferred to delete the move constructor and move operator= but unfortunately those are needed for our unused http push_block.

Resolves #737

… still might be in use. For example if still in the forkdb. Would have preferred to `delete` the move constructor/operator= but unfortunately those are needed for our unused http push_block.
@heifner heifner added the OCI Work exclusive to OCI team label Sep 10, 2024
@heifner heifner linked an issue Sep 10, 2024 that may be closed by this pull request
Copy link
Member

@linh2931 linh2931 left a comment

Choose a reason for hiding this comment

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

So this IS a bug as push_block in chain_plugin.cpp moves the signed block parm.

@heifner
Copy link
Member Author

heifner commented Sep 10, 2024

So this IS a bug as push_block in chain_plugin.cpp moves the signed block parm.

What push_block moves has only been seen by HTTP, so it is safe to move that one.

@heifner
Copy link
Member Author

heifner commented Sep 10, 2024

The asan test is still failing after this change. But I do think this is still a good change as we access QC's from blocks in the forkdb. At least until #719 is merged.

@ericpassmore ericpassmore added the test-instability tag issues for flaky tests, high priority to address label Sep 10, 2024
@ericpassmore
Copy link
Contributor

Note:start
category: Other
component: Plugins/Cleos/Utils
summary: Do not move a signed block that may be used later.
Note:end

@greg7mdp
Copy link
Contributor

@heifner, should you close this PR?

@heifner
Copy link
Member Author

heifner commented Sep 11, 2024

@heifner, should you close this PR?

Still need this PR. The clone() is how you make the copies. The other PR #744 is targeted to it and provides the mechanism that enforces clone() has to be called instead of std::move(*b).

@heifner heifner changed the title [1.0.1] Should never move from a signed_block [1.0.1] Should never move from a signed_block in a shared_ptr Sep 11, 2024
@heifner heifner merged commit bb12b12 into release/1.0 Sep 11, 2024
36 checks passed
@heifner heifner deleted the GH-737-move-block branch September 11, 2024 23:04
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 test-instability tag issues for flaky tests, high priority to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: asan block_unit_test_eos-vm-oc
6 participants