chore: narrow fork_database bmi using (F020) + pin submodules (F032)#123
Merged
Conversation
Header hygiene (reduce namespace pollution leaked to every consumer): - F020 fork_database.hpp: replace 'using namespace boost::multi_index;' with the 7 specific names the index typedef uses. Verified no consumer relies on the leak. - F023/F024 dlt_block_log.hpp / block_log.hpp: replace 'using namespace graphene::protocol;' with the only two names used (signed_block, optional). Submodules (F032): drop 'branch =' from .gitmodules so a stray 'submodule update --remote' cannot silently advance the pinned commits. CI/Docker use 'submodule update --init --recursive' (recorded SHAs), so build behaviour is unchanged. Deliberately NOT touched (load-bearing, the audit's 'move to .cpp' is wrong for these): - F021 chain_object_types.hpp 'using namespace boost::multi_index;' is relied on by 13+ object headers (account_object.hpp uses 58 bare bmi tokens with no using of its own). Removing it cascade-breaks the object layer; it needs per-consumer usings added first. - F022 operation_util_impl.hpp: same risk class (broad visitor include).
The previous commit narrowed the blanket protocol using out of block_log.hpp / dlt_block_log.hpp, but block_log.cpp and dlt_block_log.cpp relied on that header leak for block_id_type and block_header. Move the using into the .cpp files (the audit's intent): headers stay minimal, the .cpp gets the full protocol namespace it implements against. Verified other consumers (database.hpp via fork_database.hpp/chain_object_types.hpp, p2p_plugin.cpp via its own usings) do not depend on the removed leak.
block_log.hpp / dlt_block_log.hpp are pulled in transitively via database.hpp across most of the codebase. Their 'using namespace graphene::protocol;' leaks the whole protocol namespace into graphene::chain, and far-flung consumers (operation_history/plugin.hpp needs annotated_signed_transaction, json_rpc/utility.hpp, ...) rely on that leak. Narrowing it cascades into widespread build breakage that can't be fixed without a per-consumer using audit — the same load-bearing situation as F021/F022, just transitive. Keeping only the genuinely-safe items in this PR: - F020 fork_database.hpp (bmi names still provided namespace-wide by chain_object_types.hpp, so no breakage) - F032 submodule pinning (no compile impact).
chiliec
changed the title
Member
Author
|
Убрал строки |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hygiene sweep from the tech-debt audit. Scope narrowed after CI revealed load-bearing leaks — see below.
Done (verified green)
fork_database.hpp—using namespace boost::multi_index;→ the 7 specific names the index typedef uses. Safe because bmi names are still provided namespace-wide bychain_object_types.hpp, so no downstream consumer loses them..gitmodules— dropbranch =lines so a straygit submodule update --remotecannot silently advance the pinned commits. CI/Docker usesubmodule update --init --recursive(recorded SHAs), so build behaviour is unchanged.Attempted, then reverted (load-bearing transitive leak)
block_log.hpp/dlt_block_log.hpp— narrowing theirusing namespace graphene::protocol;broke the build. These headers are pulled in transitively viadatabase.hppacross most of the codebase; far-flung consumers (operation_history/plugin.hppneedsannotated_signed_transaction,json_rpc/utility.hpp, …) rely on the leak. Reverted after two CI cycles of whack-a-mole.Not attempted (same load-bearing class)
chain_object_types.hppbmi using — relied on by 13+ object headers (account_object.hppuses 58 bare bmi tokens with no own using).operation_util_impl.hpp— broad operation-visitor include.new/delete: descoped (pimpl.reset(new X())has no benefit;fc::promise::ptrcan't use make_unique). F026 already fixed.Lesson recorded: removing
using namespacefrom any header reachable viadatabase.hppis a major refactor, not a sweep. Only leaf headers (F018 wallet.hpp) and redundantly-provided names (F020) are safe to narrow.