merge from dev - #227
Merged
Merged
merge from dev#227
Conversation
Three related logging defects made master node list failures undiagnosable.
blockchain.cpp used a fmt-style "{}" placeholder inside a stream-style MFATAL,
so the fatal error that stops the daemon printed a literal "{}" instead of the
block that failed:
Unable to process block {} for updating master node list: ...
crypto::hash has a non-explicit operator bool() and no fmt formatter, so
passing one to fmt::format silently selects the bool conversion and prints
"true". Seven call sites in master_node_list.cpp did this, including the
checkpoint verification failures, so they never identified a block either:
Failed to verify block components for incoming block true at height 5703039
They now go through tools::type_to_hex.
Finally, three of the paths where master_node_list::load() gives up returned
false without logging. Each one silently sends the daemon into a full rescan
from the hf9 height, so an operator had no way to tell why a start was slow.
master_node_list::store() was only called at the end of a subsystem rescan and in core::deinit(), so a daemon following the chain tip never wrote the list out until it was asked to shut down cleanly. Any unclean exit -- SIGKILL, OOM, power loss, or an assertion failure in a dependency -- therefore discarded every block of state accumulated since the process started. On the next launch load_missing_blocks_into_beldex_subsystems() then replays from wherever the last stored state left off. For a long-running daemon that is most of the chain: slow, and able to fail outright, in which case the daemon cannot start at all and the operator is left restoring the database by hand. Write the list from core::on_idle instead, at most once every five minutes and only when it has advanced, which bounds the replay after an unclean exit to a few blocks. The write takes the blockchain lock as well as the list lock and runs off the block handling path, matching cleanup_proofs(); store() is otherwise unchanged, so a clean shutdown behaves exactly as before. Verified by killing a synced master node with SIGKILL and no grace period: an unpatched daemon failed its replay and had to rebuild from a snapshot, while a checkpointing one restarted with its chain intact.
load_missing_blocks_into_beldex_subsystems() fetched each block's transactions one block at a time, on the same thread that then applies the master node list and BNS updates. Deserialising those transactions dominates the scan: the progress line typically reported around two seconds of subsystem work per ten second window, with the rest spent loading and parsing. Transaction loading is independent per block, so hand a whole chunk to the thread pool and keep the subsystem updates strictly sequential afterwards. This mirrors how oxen-core loads blocks for the same scan. Doing so needs a non-locking accessor: get_transactions() takes the blockchain lock, so calling it from several workers would serialise them again. Split it into a locking wrapper and _get_transactions(), and have the scan hold the blockchain lock for its whole duration instead. The lock is recursive, so this is safe when a caller already holds it (pop_blocks does). The LMDB layer gives each thread its own read transaction, so concurrent reads are fine. The progress and summary lines now also report time spent loading transactions, so the split between loading and subsystem work is visible.
The periodic checkpoint called store() unchanged, which serialises the long-term state archive whenever it is dirty. That archive grows without bound -- hundreds of MB and >100k historical states on mainnet -- and the ostringstream/str()/append sequence transiently needs several times its size. On a memory-capped node that spike is fatal: a fleet of 2GB containers running the 5 minute checkpoint was OOM-killed repeatedly, with the kernel reporting anon-rss exactly at the limit. It is also reachable on a schedule rather than rarely. process_block() marks the archive dirty for 246 of every 10000 heights, and continuously while a node is catching up -- which is precisely when a freshly bootstrapped node is already at its memory ceiling. Give store() an include_archive parameter, default true, and have only the periodic caller pass false. The dirty flag is then left set so the next full store() -- end of rescan, or core::deinit() -- still writes the archive. The three existing callers are unchanged, so shutdown and rescan behaviour is identical. Recovery is unaffected: load() restores m_state from the short-term blob and never from the archive. Also fix two things found alongside it: blockchain_detached() moved m_state backwards without clearing m_last_stored_height, so checkpoint_state()'s guard suppressed every write until the chain climbed back past the old high-water mark. A state written for an abandoned fork could survive at the same height and be reloaded on the next start. Clear the mark on detach. Reduce the rescan chunk from 1000 blocks to 200. Now that a chunk's transactions are parsed up front and held together, the chunk size bounds the peak allocation of the scan; oxen-core uses 50 for the same reason.
store() built the serialised blob through three live copies: the archiver's ostringstream buffer, the string that str() returns, and a member string that append() copied it into again -- all held simultaneously, on top of the already-materialised serialisation cache. For the long-term archive, which is 582MB and ~139k states on a mainnet node, that is well over a gigabyte of transient allocation, and it is what pushed 2GB nodes into swap and then into the OOM killer. Scope the archiver so its buffer is released before the database write, move the blob out instead of appending it, and drop the serialisation cache once the bytes exist. The blob is now a local, so nothing survives the call. The member it replaces, cache_data_blob, was worse than an extra copy: clear() does not release capacity, so after one archive write every node held the archive's full size in string capacity for the rest of the process's life. Also log the size and duration of each write. Until now the only way to find out what a store() cost was to measure the gap between two unrelated log lines.
Blockchain::init() runs the detach hooks at the current tip on every start. Nothing has actually been detached there -- the call exists to drive the subsystem rescan -- but blockchain_detached() treated it as a real reorg. The list that load() restores always trails the tip, because store() only serialises states up to the short-term cull window, so the revert target was never present in state_history. That sent the function to its fallback, which rewinds to the previous STORE_LONG_TERM_STATE_INTERVAL boundary and discards every state since: up to 10k blocks of valid work thrown away on a clean restart. The replay that followed was not merely slow. It re-verifies state change votes against quorums it rebuilds as it goes, and one block it cannot re-verify aborts startup with "Failed to start core". On mainnet a node restarting while that boundary sat below height 5722157 could not start at all, and recovered only by discarding its chain and resyncing. Return early when the list is already at or behind the revert point. A clean restart now rescans ~1.2k blocks in about 3s rather than ~9.3k and failing.
- Persist BNS processing progress every 1000 blocks. - Reduce unnecessary rescans after an unclean shutdown. - Roll back BNS mappings when the database is ahead of the blockchain. - Update BNS settings after rolling back to the chain tip. - Avoid dropping and rebuilding the entire BNS database.
…inting Persist the master node list periodically so an unclean exit doesn't cost the database
fix: reset cached height after blockchain recovery clear
fix: improve BNS database recovery after unclean shutdown
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
4 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.
No description provided.