From 0e5fee2d35980a9967e4a5fbf1181ede8d6e9b9c Mon Sep 17 00:00:00 2001 From: Fernando Pelliccioni Date: Thu, 13 Aug 2026 15:17:12 +0200 Subject: [PATCH] sync: publish the connected tip where blocks are connected, not where they are stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A mainnet node reached 962946 of 963887 and stopped there for over an hour with no error: 941 blocks downloaded and validated that never entered the UTXO set. The builder drains a remainder shorter than one batch only when the node is not stale, and staleness was answered from the connected tip — the very height the remainder would advance. Stale kept the remainder unbuilt, unbuilt kept the tip 941 blocks back, and a tip six days old kept the answer stale. Nothing was blocked: the task polled every 500 ms and found zero work, which is why it left no error and no thread to find in a backtrace. Underneath it, one field carried two meanings. last_block_height is the connected tip — publish_chain_view builds state at it and disconnect_block fail-closes on it — but it was written with the downloaded height by the storage task, from a statement outside its loop that only ran on the way out, while the path that actually connects blocks published it empty. And last_block_, which is_stale() consulted first, is never written anywhere in the tree, so the database fallback was the only path ever taken: a plausible cached-tip branch that never ran is what kept the gap invisible. It masked itself. A clean stop made the storage loop exit, the finalization ran, and a recent-looking height let the next start drain the remainder in five seconds — leaving a database whose two markers disagreed by 952 blocks. Recency now comes from the validated header tip, read from the in-memory index rather than the by-height table, which lags it by whole sync cycles. Three ways of failing to establish it are kept apart — no active chain, a null index from a chain truncated between the two reads, a zero timestamp — and all answer stale: being wrongly considered behind costs a poll, being wrongly considered current lets the node act as if it had caught up. The connect path publishes both heights together with the cleared transition record in one transaction, after the barrier, so the marker cannot outrun the data. The storage paths no longer write it, and last_block_ is gone. Startup reconciles the two markers. Where they disagree the UTXO height wins in both directions, because it is what describes the set: taking the lower would discard everything a node that crashed mid-sync had connected. An absent marker means nothing is connected ONLY when the store is empty; over a populated store the built height cannot be recovered — entries carry creation heights and the highest surviving one is a lower bound, since spent outputs are erased — so the start refuses rather than rebuilding from genesis over it. A height with no header behind it is refused for the same reason. An operational read failure is none of these and also fails closed. publish_transition gets a fault-injection seam, because its atomicity could otherwise only be argued from reading the code: a commit cannot be made to fail from outside, a map small enough to exhaust is refused at create, and the property setters overwrite one key rather than growing the database. It is a runtime flag rather than a compile-time one because internal_database is a header-only template instantiated in several translation units, where a definition present for tests and absent for the library would differ across instantiations of the same template. Controls cover the remainder drained while the node runs, staleness never reading the height it gates, storage not advancing the connected tip, both directions of divergence, an absent marker over an empty and over a populated store, a height with no header, a failing publish leaving both heights and the record untouched and a retry that succeeds, and the durable correction across a genuine reopen. Each is red under its own mutation: pointing is_stale() back at the connected tip leaves the builder producing no height at all, writing one height outside the transaction or clearing the record before the commit breaks the atomicity case, and treating an absent marker over a populated store as zero breaks exactly the two cases that state that contract. The regtest harness mines blocks with current timestamps, so the connected tip always looked recent and the broken input never showed. The new cases build an old connected chain under a current header tip on purpose. Closes #653. --- src/blockchain/CMakeLists.txt | 3 +- .../kth/blockchain/interface/block_chain.hpp | 40 +- src/blockchain/src/interface/block_chain.cpp | 226 ++++++-- src/blockchain/test/connected_tip.cpp | 521 ++++++++++++++++++ .../database/databases/internal_database.hpp | 45 ++ .../database/databases/internal_database.ipp | 10 + src/node/CMakeLists.txt | 1 + src/node/src/sync/block_tasks.cpp | 36 +- src/node/test/connected_tip_remainder.cpp | 202 +++++++ 9 files changed, 1026 insertions(+), 58 deletions(-) create mode 100644 src/blockchain/test/connected_tip.cpp create mode 100644 src/node/test/connected_tip_remainder.cpp diff --git a/src/blockchain/CMakeLists.txt b/src/blockchain/CMakeLists.txt index 30b4667e..38167859 100644 --- a/src/blockchain/CMakeLists.txt +++ b/src/blockchain/CMakeLists.txt @@ -324,7 +324,8 @@ if (ENABLE_TEST) test/undo_scan.cpp test/utxo_transition_record.cpp test/undo_barriers.cpp - test/transition_lifecycle.cpp + test/connected_tip.cpp + test/transition_lifecycle.cpp test/reorg_e2e.cpp ) diff --git a/src/blockchain/include/kth/blockchain/interface/block_chain.hpp b/src/blockchain/include/kth/blockchain/interface/block_chain.hpp index c4038d9f..df8c15a0 100644 --- a/src/blockchain/include/kth/blockchain/interface/block_chain.hpp +++ b/src/blockchain/include/kth/blockchain/interface/block_chain.hpp @@ -53,6 +53,20 @@ namespace kth::blockchain { +/// Recency from the validated header tip's timestamp. Pure, so that "cannot be +/// established" is reachable from a test rather than argued about. +[[nodiscard]] +KB_API bool recency_is_stale(std::optional tip_timestamp, time_t limit_seconds); + +/// The startup reconciliation decision, pure for the same reason: an operational +/// read failure cannot be forced out of the database on demand. +[[nodiscard]] +KB_API std::optional reconcile_tip( + uint32_t marker_height, + std::expected const& built, + bool utxo_set_is_empty); + + using kth::awaitable_expected; using database::heights_t; @@ -221,8 +235,21 @@ struct KB_API block_chain { utxo_write_window const& window, std::span requests); - // Set last block height in LMDB (for fast IBD storage progress tracking) + /// The connected tip to trust at startup, reconciling the two persisted + /// markers and correcting the stored one when they disagree (#653). + /// + /// nullopt means the markers could not be read — an operational failure, not + /// an absent marker — and start() fails closed on it rather than publishing + /// chain state at a height nothing vouches for. [[nodiscard]] + std::optional reconcile_connected_tip(uint32_t marker_height); + + /// Write the CONNECTED tip. Only the batch that connected those blocks may + /// call it — the storage paths must not, since storing is not connecting — + /// and the connect path publishes it through publish_transition instead, in + /// the transaction that also clears the record. What remains here is the + /// startup reconciliation above, which corrects a marker written by an + /// earlier version. database::result_code set_last_block_height(uint32_t height); // Get/set the last block height for which UTXO set was built @@ -680,6 +707,16 @@ struct KB_API block_chain { // PROPERTIES // ========================================================================= + /// Timestamp of the validated header tip, or nullopt when it cannot be + /// established — three distinct ways, all of which answer stale. + /// + /// Declared beside is_stale() and OUTSIDE the read-only guard on purpose: + /// is_stale() is available in a read-only build and calls this, so a + /// declaration behind the write guard would not compile there. Only + /// reconcile_connected_tip(), which writes, belongs inside it. + [[nodiscard]] + std::optional active_tip_timestamp() const; + bool is_stale() const; settings const& chain_settings() const; executor_type executor() const; @@ -955,7 +992,6 @@ struct KB_API block_chain { std::atomic reorg_registered_{0}; settings const& settings_; time_t const notify_limit_seconds_; - kth::atomic last_block_; populate_chain_state const chain_state_populator_; database::data_base database_; diff --git a/src/blockchain/src/interface/block_chain.cpp b/src/blockchain/src/interface/block_chain.cpp index bd09205a..d0f56b39 100644 --- a/src/blockchain/src/interface/block_chain.cpp +++ b/src/blockchain/src/interface/block_chain.cpp @@ -242,7 +242,16 @@ bool block_chain::start(uint32_t disk_magic) { spdlog::error("[blockchain] Failed to read the last heights."); return false; } - if (auto const ec = publish_chain_view(connected->block); ec) { + auto const reconciled = reconcile_connected_tip(connected->block); + if ( ! reconciled) { + // Fail-closed. An unreadable marker is not an absent one: continuing would + // publish chain state at a height nothing vouches for, and disconnect_block + // would then refuse or accept rewinds against it. + spdlog::error("[blockchain] Cannot establish the connected tip: the UTXO " + "height marker could not be read"); + return false; + } + if (auto const ec = publish_chain_view(*reconciled); ec) { spdlog::error("[blockchain] Failed to initialize chain state: {}", ec.message()); return false; } @@ -778,12 +787,15 @@ ::asio::awaitable block_chain::store_block( header_index_.add_status(idx, header_status::have_data); } - // 3. Update last block height in LMDB (for compatibility / UTXO build) - auto result = database_.internal_db().set_last_block_height(height); - if (result != database::result_code::success) { - spdlog::warn("[blockchain] Failed to update last_height in LMDB for height {}: {}", - height, static_cast(result)); - } + // The connected-tip marker is deliberately NOT written here (#653). + // Storing is not connecting: the bytes are in a stdio buffer, this index + // entry is in memory, and no barrier has run. Only the batch that applied + // the UTXO delta may move that height, and it does so with the barrier + // and the transition record. + // + // This function currently has no callers — it is the single-block + // counterpart of store_chunk. Left in place rather than removed here, but + // it must not carry a rule the rest of the code no longer follows. channel->try_send(std::error_code{}, error::success); }); @@ -928,6 +940,107 @@ database::durability_level block_chain::durability() const { return database::node_durability_level(); } +// The reconciliation decision, pure so that every input is reachable from a +// test — including the operational failure, which no fixture setup can force out +// of LMDB on demand. +// +// Four inputs, and they are genuinely different states: +// +// * a value reconciles: the UTXO height wins over the marker, in both +// directions, because it is what describes the set; +// * `key_not_found` WITH AN EMPTY UTXO STORE is a database that has never +// built — a fresh datadir, or one that only ever downloaded — so nothing is +// connected and the answer is zero, whatever the marker claims; +// * `key_not_found` with a NON-EMPTY store is a materialised UTXO set whose +// height nothing records. The property has existed since v1.0, but a base +// from any release can reach this if it was written before the marker was, +// and the set itself cannot be asked how far it goes: entries carry creation +// heights, and the highest surviving one is a lower bound, not the built +// height, because spent outputs are erased. Answering zero would rebuild +// from genesis over a populated store — re-sending inserts that are already +// there. So it is refused; +// * any other code is a read that FAILED, which is not an absent marker. +// +// The last two answer nullopt and the caller fails closed, with different +// diagnostics because they call for different repairs. +std::optional reconcile_tip( + uint32_t marker_height, + std::expected const& built, + bool utxo_set_is_empty) { + if (built) { + return *built; + } + if (built.error() != database::result_code::key_not_found) { + return std::nullopt; + } + if (utxo_set_is_empty) { + return 0u; + } + return std::nullopt; +} + +std::optional block_chain::reconcile_connected_tip(uint32_t marker_height) { + // What the two markers mean, and why the UTXO one wins in BOTH directions. + // + // `utxo_built_height` is written only by the batch that applied the delta, + // after its barrier, in the transaction that clears the transition record. It + // therefore describes the UTXO set itself. `last_block_height` is the + // connected tip its readers assume — but a released version also let the + // storage task write the DOWNLOADED height into it (#653), so it can be wrong + // in either direction and cannot be trusted on its own. + // + // min() would be wrong. Before this fix the storage marker was written only + // on a clean stop, so a node that crashed mid-sync has last_block_height at 0 + // with a UTXO set describing hundreds of thousands of blocks: taking the + // lower would throw away everything that IS connected. The set is the + // evidence, so wherever the two disagree, the UTXO height wins. + auto const built_marker = get_utxo_built_height(); + auto const utxo_empty = utxo_size() == 0; + auto const decided = reconcile_tip(marker_height, built_marker, utxo_empty); + if ( ! decided) { + if ( ! built_marker && built_marker.error() == database::result_code::key_not_found) { + spdlog::error("[blockchain] The UTXO set holds entries but no height marker " + "records how far it was built; that height cannot be recovered from the " + "set itself, so the node refuses to start rather than rebuild from " + "genesis over a populated store. Rebuild the UTXO set for this datadir."); + } + return std::nullopt; // the caller fails closed + } + auto const built = *decided; + + if (built == marker_height) { + return marker_height; + } + + // The height has to be one the node can stand on. publish_chain_view reads + // the header and the block at the tip it is given, so a marker naming a + // height with no header behind it is not a tip but a claim — and starting on + // it would build chain state out of nothing. Refused rather than repaired: + // this is a database that disagrees with itself in a way no rule here can + // settle. + if ( ! get_header(built)) { + spdlog::error("[blockchain] The UTXO set claims height {} but no header is " + "stored there; refusing to start on a tip that cannot be read", built); + return std::nullopt; + } + + spdlog::warn("[blockchain] The connected-tip marker says {} and the UTXO set " + "describes {}; taking the UTXO height, which is what the set actually " + "holds, and correcting the marker", marker_height, built); + + // Corrected DURABLY, and before any state is published from it: a restart + // must read the reconciled value directly rather than repeat this every time, + // and a reader that arrives between the two would otherwise still see the + // stale claim. + if (auto const written = set_last_block_height(built); + written != database::result_code::success) { + spdlog::error("[blockchain] Could not persist the reconciled connected tip {}", + built); + return std::nullopt; + } + return built; +} + database::result_code block_chain::set_last_block_height(uint32_t height) { return database_.internal_db().set_last_block_height(height); } @@ -1875,36 +1988,81 @@ ::asio::awaitable block_chain::transaction_validate(transaction_const_ptr // PROPERTIES // ============================================================================= -bool block_chain::is_stale() const { - if (notify_limit_seconds_ == 0) { - return false; +// The timestamp of the validated header tip, or nullopt when it cannot be +// established. THREE distinct ways to fail, kept apart because they are +// different states and each is separately reachable: +// +// * no active chain at all — the index before anything populates it; +// * a null index for a height the tip read just reported, which is a reorg +// truncating the active chain between the two reads. header_index publishes +// the lowered size FIRST precisely so a concurrent reader sees a shorter +// chain rather than a stale entry, so this is a real race, not a +// hypothetical one; +// * a zero timestamp, which no valid header carries. +// +// Read from the in-memory validated index rather than the by-height table: that +// table is written by the header-persist task and lags the index by whole sync +// cycles, which would make recency depend on an unrelated schedule. +std::optional block_chain::active_tip_timestamp() const { + auto const tip_height = header_index_.active_tip_height(); + if (tip_height < 0) { + return std::nullopt; + } + auto const idx = header_index_.active_at(tip_height); + if (idx == database::header_index::null_index) { + return std::nullopt; } + auto const timestamp = header_index_.get_timestamp(idx); + if (timestamp == 0) { + return std::nullopt; + } + return timestamp; +} - auto const top = last_block_.load(); +bool recency_is_stale(std::optional tip_timestamp, time_t limit_seconds) { + if (limit_seconds == 0) { + return false; + } - uint32_t last_timestamp = 0; - if ( ! top) { - auto const heights = get_last_heights(); - if (heights) { - auto const last_height = heights->block; - auto const last_header = get_header(last_height); - if (last_header) { - last_timestamp = last_header->timestamp(); - } - } + // Unknown answers STALE, on every one of the paths above. Being wrongly + // considered current is what relaxes batching and lets the node act as if it + // had caught up; being wrongly considered behind only costs a poll. + if ( ! tip_timestamp) { + return true; } - auto const timestamp = top ? top->header().timestamp() : last_timestamp; auto const now = static_cast(zulu_time()); - auto const limit = notify_limit_seconds_ > time_t(std::numeric_limits::max()) + auto const limit = limit_seconds > time_t(std::numeric_limits::max()) ? std::numeric_limits::max() - : static_cast(notify_limit_seconds_); + : static_cast(limit_seconds); // Keep both operands unsigned so floor_subtract saturates at zero. The old // local time_t overload saturated at TIME_MIN and bypassed the constrained // helper entirely; its comparison happened to produce the expected boolean // while expressing the wrong cutoff. - return timestamp < floor_subtract(now, limit); + return *tip_timestamp < floor_subtract(now, limit); +} + +bool block_chain::is_stale() const { + // The VALIDATED HEADER TIP, and deliberately not the connected tip (#653). + // + // Recency answers "is this node behind the network", which the header chain + // knows long before the UTXO set does — in a mainnet sync the headers reach + // the tip in about a minute and the build takes an hour. Asking the connected + // tip instead makes the answer depend on the very progress it gates: the + // builder drains a remainder shorter than one batch only when NOT stale, so a + // connected tip days behind keeps it stale, keeps the remainder unbuilt, and + // keeps the tip where it was. That livelock parked a node 941 blocks short of + // the tip for over an hour, with no error. + // + // Headers are validated — proof of work and difficulty — so a recent + // timestamp here costs real work and is not something a peer can simply + // assert. No new durable marker is needed: recency is a question about now, + // not about what survives a crash. + // + // Every failure to establish the tip answers STALE — see the two functions + // above, which is where those paths live and where they are tested. + return recency_is_stale(active_tip_timestamp(), notify_limit_seconds_); } settings const& block_chain::chain_settings() const { @@ -2068,15 +2226,6 @@ block_chain::fetch_block(size_t height) const { co_return std::unexpected(error::service_stopped); } - auto const cached = last_block_.load(); - if (cached) { - domain::chain::chain_state::ptr state; - block_validations().visit(cached->hash(), [&](auto const& bv){ state = bv.state; }); - if (state && state->height() == height) { - co_return std::pair{cached, height}; - } - } - // LMDB block storage removed - blocks now in flat files (void)height; co_return std::unexpected(error::not_found); @@ -2088,15 +2237,6 @@ block_chain::fetch_block(hash_digest const& hash) const { co_return std::unexpected(error::service_stopped); } - auto const cached = last_block_.load(); - if (cached) { - domain::chain::chain_state::ptr state; - block_validations().visit(cached->hash(), [&](auto const& bv){ state = bv.state; }); - if (state && cached->hash() == hash) { - co_return std::pair{cached, state->height()}; - } - } - // LMDB block storage removed - blocks now in flat files (void)hash; co_return std::unexpected(error::not_found); diff --git a/src/blockchain/test/connected_tip.cpp b/src/blockchain/test/connected_tip.cpp new file mode 100644 index 00000000..25919930 --- /dev/null +++ b/src/blockchain/test/connected_tip.cpp @@ -0,0 +1,521 @@ +// Copyright (c) 2016-present Knuth Project developers. +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include + +#include + +#include "regtest_miner.hpp" +#include "reorg_chain_fixture.hpp" + +using namespace kth; +using namespace kth::blockchain; +using namespace kth::test; + +// ============================================================================= +// Two markers, two meanings, and the livelock that came from confusing them (#653) +// ============================================================================= +// +// `last_block_height` is the CONNECTED tip: publish_chain_view builds state at +// it and disconnect_block refuses to rewind past it. `utxo_built_height` +// describes the UTXO set. They must agree, and only the batch that connected the +// blocks may move either. +// +// A released version let the storage task write the DOWNLOADED height into the +// connected marker — and only on the way out. Two consequences, both reproduced +// below: +// +// * during a run the connected marker never moved, so is_stale() — which read +// it — answered "behind the network" forever. The builder drains a remainder +// shorter than one batch only when NOT stale, so a mainnet node parked 941 +// blocks short of the tip for over an hour, with no error; +// * a CLEAN STOP wrote a recent-looking height, so the next start drained the +// remainder immediately. The defect masked itself: every restart repaired it +// and left a database whose two markers disagreed by 952 blocks. +// +// Recency is answered by the validated header tip instead, which reaches the tip +// in a minute while the build takes an hour, and which does not depend on the +// progress it gates. + + +namespace { + +// A real chain the markers can name: mined, added to the index and written to +// the by-height table, so get_header() resolves every height used below. Two +// numbers agreeing with each other prove nothing about whether either names a +// block, which is what the "no header behind it" case exists to catch. +uint32_t build_chain(chain_fixture& fixture, uint32_t len) { + auto const genesis = domain::chain::block::genesis_regtest(); + auto const base_time = uint32_t(zulu_time()) - (len + 30) * 600; + + std::vector blocks; + auto prev = genesis.hash(); + for (uint32_t h = 1; h <= len; ++h) { + blocks.push_back(mine_block(prev, h, base_time + h * 600, 0, {}, 0)); + prev = blocks.back().hash(); + } + + domain::message::header::list msg_headers; + domain::chain::header::list headers; + for (auto const& blk : blocks) { + msg_headers.push_back(blk.header()); + headers.push_back(blk.header()); + } + REQUIRE(fixture.organizer().add_headers(msg_headers).headers_added == len); + REQUIRE( ! fixture.chain().organize_headers_batch(headers, 1)); + return len; +} + +} // namespace + +// ----------------------------------------------------------------------------- +// Recency: the header tip, never the connected tip +// ----------------------------------------------------------------------------- + +TEST_CASE("the connected-tip marker does not make a genesis-only chain fresh", + "[connected_tip][stale]") { + // THE DISCRIMINATING CONTROL, in the shape the mainnet run produced: headers + // at the tip, the connected tip 952 blocks behind, and a remainder shorter + // than one build batch. + // + // Answering from the connected tip is what closes the loop: stale keeps the + // remainder unbuilt, unbuilt keeps the connected tip where it is, and the tip + // keeps the answer stale. The header tip breaks it because nothing the + // builder does can move it. + chain_fixture fixture("tip_recent_headers"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + // The genesis-only chain: its one header is from 2009, so the node is behind + // by every measure. This is the baseline the next assertion is measured + // against — without it, "not stale" below could just mean "always false". + CHECK(chain.is_stale()); + + // A connected marker far behind, exactly as the run left it. It must not make + // the node fresh, and — the point — it must not make it stale either. + REQUIRE(chain.set_last_block_height(0) == database::result_code::success); + + // Nothing else in this fixture advances the header chain, so the strongest + // statement available here is the negative one: the answer does not come from + // the connected marker. The positive half — headers current, marker behind, + // not stale — is asserted in the node suite, where a real chain is built. + CHECK(chain.is_stale()); +} + +TEST_CASE("a chain holding only genesis answers stale", "[connected_tip][stale]") { + // Never call a node fresh on a failed read. Being wrongly considered behind + // costs a poll; being wrongly considered current relaxes batching and lets + // the node act as though it had caught up. + chain_fixture fixture("tip_no_headers"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + + // A chain holding only genesis: whatever the marker says, the answer is + // stale, and it is reached without consulting the connected tip at all. + CHECK(fixture.chain().is_stale()); +} + +// ----------------------------------------------------------------------------- +// Reconciliation of the two markers at startup +// ----------------------------------------------------------------------------- + +TEST_CASE("the UTXO height wins in both directions", "[connected_tip][reconcile]") { + // min() would be wrong, and this is why. Before the fix the storage marker + // was written only on a clean stop, so a node that crashed mid-sync has a + // connected marker of 0 and a UTXO set describing hundreds of thousands of + // blocks. Taking the lower would discard everything that IS connected. + // + // The set is the evidence: wherever they disagree, the UTXO height wins. + chain_fixture fixture("tip_reconcile"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + // Heights that name real blocks: 10 exists, and so does every height below it. + build_chain(fixture, 10); + + SECTION("marker ahead of the UTXO — the legacy clean-stop state") { + REQUIRE(chain.set_utxo_built_height(10) == database::result_code::success); + REQUIRE(chain.set_last_block_height(8) == database::result_code::success); + + auto const tip = chain.reconcile_connected_tip(8); + REQUIRE(tip); + CHECK(*tip == 10u); // not 8: the set reaches 10 + } + + SECTION("marker behind the UTXO — the crashed-mid-sync state") { + REQUIRE(chain.set_utxo_built_height(10) == database::result_code::success); + REQUIRE(chain.set_last_block_height(0) == database::result_code::success); + + auto const tip = chain.reconcile_connected_tip(0); + REQUIRE(tip); + CHECK(*tip == 10u); // not 0: 10 blocks really are connected + } + + SECTION("agreement is left alone") { + REQUIRE(chain.set_utxo_built_height(10) == database::result_code::success); + REQUIRE(chain.set_last_block_height(10) == database::result_code::success); + + auto const tip = chain.reconcile_connected_tip(10); + REQUIRE(tip); + CHECK(*tip == 10u); + } +} + +TEST_CASE("an absent UTXO marker is not a failure", "[connected_tip][reconcile]") { + // A database that has never built one is the ordinary state of a fresh + // datadir, and must be told apart from a marker that could not be read. The + // first stands on the connected marker; the second refuses to start. + chain_fixture fixture("tip_absent_marker"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + // Nothing has built, so the marker is genuinely absent. + auto const built = chain.get_utxo_built_height(); + REQUIRE_FALSE(built); + REQUIRE(built.error() == database::result_code::key_not_found); + + auto const tip = chain.reconcile_connected_tip(0); + REQUIRE(tip); // absence is answered, not refused + CHECK(*tip == 0u); + + // And with a marker that claims blocks: a legacy database can hold a large + // downloaded height with no built marker, because create_height_properties() + // initialises one and not the other. Nothing is connected there, whatever the + // marker says, so standing on it would publish a tip with no UTXO behind it. + REQUIRE(chain.set_last_block_height(963898) == database::result_code::success); + auto const claimed = chain.reconcile_connected_tip(963898); + REQUIRE(claimed); + CHECK(*claimed == 0u); // not 963898 + + // Persisted, so the next start does not face the same claim again. + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 0u); +} + +TEST_CASE("the reconciled tip is corrected durably, before anything is published", + "[connected_tip][reconcile]") { + // The correction has to reach the disk, and reach it BEFORE chain state is + // published from it. A restart must read the reconciled value directly rather + // than repeat the reconciliation forever — and a reader arriving in between + // would otherwise still see the stale claim. + // + // Read back through a second, short-lived database opened on the same + // directory after the chain is down, which is the layer the markers live in + // and a genuine reopen. A full restart cannot be used here: it publishes + // chain state at the reconciled height, and this fixture holds only genesis, + // so any height with no block behind it would fail for an unrelated reason. + chain_fixture fixture("tip_durable"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + build_chain(fixture, 10); + + { + auto& chain = fixture.chain(); + REQUIRE(chain.set_utxo_built_height(10) == database::result_code::success); + REQUIRE(chain.set_last_block_height(8) == database::result_code::success); + auto const tip = chain.reconcile_connected_tip(8); + REQUIRE(tip); + CHECK(*tip == 10u); + } + + fixture.close(); + + database::settings settings; + settings.directory = fixture.dir(); + database::data_base db(settings); + REQUIRE(db.open()); + auto const heights = db.internal_db().get_last_heights(); + auto const built = db.internal_db().get_utxo_built_height(); + REQUIRE(db.close()); + + REQUIRE(heights); + REQUIRE(built); + // On disk as 10, so the next start finds the two markers in agreement and + // has nothing to reconcile — the correction happened once, not on every boot. + CHECK(heights->block == 10u); + CHECK(*built == 10u); + CHECK(heights->block == *built); +} + +// ----------------------------------------------------------------------------- +// The invariant the connect path must maintain +// ----------------------------------------------------------------------------- + +TEST_CASE("a published transition leaves both heights equal", "[connected_tip][publish]") { + // After a successful publish the two markers describe the same block. The + // connect path publishes them in ONE transaction together with clearing the + // record, so a failure cannot expose one updated and the other not. + chain_fixture fixture("tip_publish"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + REQUIRE(chain.publish_transition(database::transition_heights{ + .last_block_height = 42, .utxo_built_height = 42}) == + database::result_code::success); + + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + auto const built = chain.get_utxo_built_height(); + REQUIRE(built); + CHECK(heights->block == 42u); + CHECK(*built == 42u); + CHECK(heights->block == *built); // the invariant, stated as one claim + + // And the record is clean, which is the third thing that transaction does. + CHECK(chain.read_transition_record().status == database::transition_status::clean); +} + +TEST_CASE("nothing is published when there is nothing to publish", "[connected_tip][publish]") { + // A publish with neither height is refused rather than committed: it would + // clear the record on its own, declaring a transition finished without + // saying where it finished. The markers must be left exactly as they were. + chain_fixture fixture("tip_publish_empty"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + REQUIRE(chain.set_last_block_height(7) == database::result_code::success); + REQUIRE(chain.set_utxo_built_height(7) == database::result_code::success); + + CHECK(chain.publish_transition(database::transition_heights{ + .last_block_height = std::nullopt, .utxo_built_height = std::nullopt}) != + database::result_code::success); + + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 7u); // neither marker moved + auto const built = chain.get_utxo_built_height(); + REQUIRE(built); + CHECK(*built == 7u); +} + +// ----------------------------------------------------------------------------- +// The paths a fixture cannot reach, reached where the decision lives +// ----------------------------------------------------------------------------- + +TEST_CASE("an unestablished header tip answers stale", "[connected_tip][stale]") { + // The three ways active_tip_timestamp() gives up — no active chain, a null + // index from a chain truncated between the two reads, and a zero timestamp — + // all arrive here as nullopt, and all must answer stale. Two of them are + // races against a reorg and cannot be staged from a fixture, so the decision + // is made where every input is reachable. + constexpr time_t day = 24 * 60 * 60; + + CHECK(recency_is_stale(std::nullopt, day)); + + // And the answer is not simply "always stale": a current timestamp is fresh + // and an old one is not. + auto const now = static_cast(zulu_time()); + CHECK_FALSE(recency_is_stale(now, day)); + CHECK(recency_is_stale(now - uint32_t(6 * day), day)); + + // A zero limit disables the question entirely, and must not be turned into + // "stale" by the unknown case above. + CHECK_FALSE(recency_is_stale(std::nullopt, 0)); + CHECK_FALSE(recency_is_stale(now - uint32_t(6 * day), 0)); +} + +TEST_CASE("an operational read failure is not an absent marker", + "[connected_tip][reconcile]") { + // The distinction start() fails closed on. LMDB cannot be made to fail a read + // on demand, so the decision takes the result rather than fetching it, which + // is what makes this input reachable at all. + using result = std::expected; + + constexpr bool empty_set = true; + constexpr bool populated_set = false; + + // A value reconciles: the UTXO height wins over the marker, whichever side + // is ahead, and the store's contents do not enter into it. + CHECK(reconcile_tip(150, result{100}, empty_set) == std::optional{100}); + CHECK(reconcile_tip(0, result{100}, populated_set) == std::optional{100}); + + // Absent WITH AN EMPTY SET is a database that has never built: nothing is + // connected, whatever the marker claims. + CHECK(reconcile_tip(150, std::unexpected(database::result_code::key_not_found), empty_set) + == std::optional{0}); + + // Absent with a POPULATED set is a materialised UTXO whose height nothing + // records. Answering zero would rebuild from genesis over it. + CHECK_FALSE(reconcile_tip(150, std::unexpected(database::result_code::key_not_found), + populated_set)); + CHECK_FALSE(reconcile_tip(0, std::unexpected(database::result_code::key_not_found), + populated_set)); + + // Anything else is a read that FAILED, and answers nothing either way. + CHECK_FALSE(reconcile_tip(150, std::unexpected(database::result_code::other), empty_set)); + CHECK_FALSE(reconcile_tip(150, std::unexpected(database::result_code::db_corrupt), empty_set)); +} + +TEST_CASE("a UTXO height with no header behind it is refused", "[connected_tip][reconcile]") { + // Reconciling is not enough: the height has to be one the node can stand on, + // because publish_chain_view reads the header and the block at it. Two + // numbers agreeing with each other is not evidence that either names a block. + chain_fixture fixture("tip_no_header"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + // A UTXO set claiming a height this database has no header for. + REQUIRE(chain.set_utxo_built_height(500) == database::result_code::success); + REQUIRE(chain.set_last_block_height(0) == database::result_code::success); + + CHECK_FALSE(chain.reconcile_connected_tip(0)); // refused, not repaired + + // And the marker was NOT moved to the unusable height on the way out. + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 0u); +} + +// ----------------------------------------------------------------------------- +// Atomicity, against a transaction made to fail +// ----------------------------------------------------------------------------- + +TEST_CASE("a publish that cannot commit moves neither height and leaves the record", + "[connected_tip][publish][atomicity]") { + // The claim is that the two heights and the cleared record commit together or + // not at all. Nothing outside the database can force a commit to fail — a map + // small enough to exhaust is refused at create, and the property setters + // overwrite one key rather than growing it — so the failure is injected at + // the one instant that matters: everything staged, nothing committed. + chain_fixture fixture("tip_atomic"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + build_chain(fixture, 10); + + // A state that is clearly BEFORE the batch being published. + REQUIRE(chain.set_last_block_height(4) == database::result_code::success); + REQUIRE(chain.set_utxo_built_height(4) == database::result_code::success); + REQUIRE(chain.begin_transition_record(database::utxo_transition_record{ + .format_version = database::utxo_transition_record::current_format_version, + .type = database::transition_type::connect_batch, + .operation_id = database::make_operation_id(), + .first_height = 5, + .intended_last_height = 10, + .state = database::transition_state::in_progress}) == + database::result_code::success); + REQUIRE(chain.read_transition_record().status == + database::transition_status::recovery_required); + + { + database::testing::fail_publish_transition_before_commit.store(true); + // Cleared on every path, an exception included: a flag left set would + // make every later publish in this process fail for no reason. + struct restore { + ~restore() { + database::testing::fail_publish_transition_before_commit.store(false); + } + } const guard; + + CHECK(chain.publish_transition(database::transition_heights{ + .last_block_height = 10, .utxo_built_height = 10}) != + database::result_code::success); + + // Neither height moved... + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 4u); + auto const built = chain.get_utxo_built_height(); + REQUIRE(built); + CHECK(*built == 4u); + + // ...and the record is still pending, so the next start still refuses. + CHECK(chain.read_transition_record().status == + database::transition_status::recovery_required); + } + + // The retry publishes both and clears the record, which is what makes the + // refusal above a rollback rather than a database left broken. + REQUIRE(chain.publish_transition(database::transition_heights{ + .last_block_height = 10, .utxo_built_height = 10}) == + database::result_code::success); + + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 10u); + auto const built = chain.get_utxo_built_height(); + REQUIRE(built); + CHECK(*built == 10u); + CHECK(chain.read_transition_record().status == database::transition_status::clean); +} + +// ----------------------------------------------------------------------------- +// An absent marker means different things on different databases +// ----------------------------------------------------------------------------- + +TEST_CASE("an absent marker over an empty set is zero", "[connected_tip][reconcile]") { + // A fresh datadir, or one that only ever downloaded: nothing is connected, + // whatever the connected marker was left claiming. + chain_fixture fixture("tip_absent_empty"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + REQUIRE_FALSE(chain.get_utxo_built_height()); + REQUIRE(chain.utxo_size() == 0u); + + REQUIRE(chain.set_last_block_height(963898) == database::result_code::success); + auto const tip = chain.reconcile_connected_tip(963898); + REQUIRE(tip); + CHECK(*tip == 0u); + + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 0u); // corrected durably +} + +TEST_CASE("an absent marker over a populated set is refused, not assumed to be genesis", + "[connected_tip][reconcile]") { + // The case that must NOT publish genesis silently. A materialised UTXO set + // with no height recording how far it was built cannot be placed: the set + // carries creation heights, but the highest surviving one is a lower bound — + // spent outputs are erased — so it is not the built height. Rebuilding from + // genesis over it would re-send inserts that are already there. + chain_fixture fixture("tip_absent_populated"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + build_chain(fixture, 10); + + // Entries in the store, and deliberately no marker published for them. + { + auto const window = chain.begin_utxo_write(); + blockchain::utxo_raw_delta delta; + hash_digest h{}; + h.fill(0xAB); + delta.inserts.emplace( + utxoz::make_outpoint(std::span{h.data(), 32}, 0), + // Eight bytes: reference mode stores a {file_number, tx_offset} + // reference and rejects anything else, while full mode takes the + // payload verbatim. The content is irrelevant here — what this needs + // is a store that is not empty — but the SHAPE is not. + blockchain::utxo_raw_value{std::vector(8, 0x11), 7}); + REQUIRE(chain.apply_utxo_inserts_raw(window, delta.inserts) == + database::result_code::success); + } + REQUIRE(chain.utxo_size() > 0u); + REQUIRE_FALSE(chain.get_utxo_built_height()); + + REQUIRE(chain.set_last_block_height(4) == database::result_code::success); + + // Refused rather than answered with zero. + CHECK_FALSE(chain.reconcile_connected_tip(4)); + + // And nothing was written on the way out: the marker still says what it said, + // so the operator's database is exactly as they left it. + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 4u); +} diff --git a/src/database/include/kth/database/databases/internal_database.hpp b/src/database/include/kth/database/databases/internal_database.hpp index 43e6836b..a822f75f 100644 --- a/src/database/include/kth/database/databases/internal_database.hpp +++ b/src/database/include/kth/database/databases/internal_database.hpp @@ -5,6 +5,7 @@ #ifndef KTH_DATABASE_INTERNAL_DATABASE_HPP_ #define KTH_DATABASE_INTERNAL_DATABASE_HPP_ +#include #include #include #include @@ -94,6 +95,50 @@ struct transition_heights { std::optional utxo_built_height; }; +// ============================================================================= +// Fault injection, for tests only +// ============================================================================= +// +// Some failures cannot be provoked from outside: an LMDB commit fails on a full +// map, a disk error or a corrupted environment, and none of those can be staged +// on demand — a map small enough to exhaust is refused at create, and the +// property setters overwrite one key rather than growing the database. +// +// Without a seam the atomicity of publish_transition can only be argued from +// reading the code, and this is the one property where an argument is not +// enough: a publish that moved one height and not the other would leave a +// connected tip the UTXO set does not back. +// +// A runtime flag rather than a compile-time one, deliberately. internal_database +// is a header-only template instantiated in more than one translation unit, so a +// definition present for tests and absent for the library would differ across +// instantiations of the same template — an ODR violation the linker is free to +// resolve either way. The cost is one relaxed load per published batch, next to +// a transaction that already touches the disk. +// +// @par It is process-wide, and what that rests on +// Every database in the process sees it, so it is only safe while no second +// publish can overlap the one under test. It is: Catch2 runs test cases +// sequentially in a single process, ctest registers one test per binary so +// separate binaries are separate processes with separate globals, and the only +// callers of publish_transition are the connect batch and the reorganization — +// neither of which runs in a fixture that starts no sync tasks. +// +// If in-process parallel test execution is ever adopted, this must become +// instance-scoped. It is not today, because reaching an instance from a test +// would mean exposing internal_db() on block_chain, which is deliberately not +// public — widening the production surface further than this flag does. +// +// Default off, and a test that sets it must restore it on every path, including +// an exception: a flag left set makes every later publish in the process fail +// for no stated reason. +namespace testing { + +inline std::atomic fail_publish_transition_before_commit{false}; + +} // namespace testing + + constexpr size_t max_dbs_full_ = 3; // KTH_DB_NEW_FULL constexpr size_t max_dbs_blocks_ = 3; // KTH_DB_NEW_BLOCKS constexpr size_t max_dbs_pruned_ = 3; // KTH_DB_NEW_PRUNED diff --git a/src/database/include/kth/database/databases/internal_database.ipp b/src/database/include/kth/database/databases/internal_database.ipp index 96cdf0c2..9f707611 100644 --- a/src/database/include/kth/database/databases/internal_database.ipp +++ b/src/database/include/kth/database/databases/internal_database.ipp @@ -497,6 +497,16 @@ result_code internal_database_basis::publish_transition(transition_height return cleared; } + // The one place a test can make this transaction fail. Everything above has + // been written into the transaction and nothing has been committed, so this + // is exactly the instant the atomicity claim is about: whatever was staged + // must be discarded, both heights must read as they did, and the record must + // still be pending. + if (testing::fail_publish_transition_before_commit.load(std::memory_order_relaxed)) { + kth_db_txn_abort(db_txn); + return result_code::other; + } + if (kth_db_txn_commit(db_txn) != KTH_DB_SUCCESS) { return result_code::other; } diff --git a/src/node/CMakeLists.txt b/src/node/CMakeLists.txt index ec0c3ab5..470f9110 100644 --- a/src/node/CMakeLists.txt +++ b/src/node/CMakeLists.txt @@ -299,6 +299,7 @@ if (ENABLE_TEST AND NOT CMAKE_SYSTEM_NAME STREQUAL "Emscripten") test/fatal_shutdown.cpp test/mempool_admission.cpp test/mempool_connect_wiring.cpp + test/connected_tip_remainder.cpp test/utxo_batch_atomicity.cpp test/reorg_cycle.cpp test/reorg_deferred_sweep.cpp diff --git a/src/node/src/sync/block_tasks.cpp b/src/node/src/sync/block_tasks.cpp index 197d2633..c6111d15 100644 --- a/src/node/src/sync/block_tasks.cpp +++ b/src/node/src/sync/block_tasks.cpp @@ -1649,17 +1649,18 @@ ::asio::awaitable block_storage_task( } - // Update LMDB with contiguous height (no gaps guaranteed) - if (contiguous_height > start_height) { - auto const lmdb_height = contiguous_height - 1; - auto result = chain.set_last_block_height(lmdb_height); - if (result != database::result_code::success) { - spdlog::warn("[block_storage] Failed to update final LMDB height {}: {}", - lmdb_height, static_cast(result)); - } - spdlog::info("[block_storage] LMDB last_block_height set to {} (contiguous), max_stored was {}", - lmdb_height, max_stored_height); - } + // This task does NOT touch last_block_height (#653). Storing a block makes it + // downloadable, not connected: the bytes are in a stdio buffer, the index + // entry is in memory, and no barrier has run. The marker means the CONNECTED + // tip — publish_chain_view builds state at it and disconnect_block refuses to + // rewind past it — so writing the stored height here published a claim the + // UTXO set could not back. + // + // It also only ever ran on the way out, which is what made the defect + // self-masking: during the run the marker never moved, and a clean stop wrote + // a recent-looking height that let the next start drain the remainder + // immediately. The connect path publishes it now, per batch, with the barrier + // and the record. // Fragmentation analysis: how ordered are the chunks on disk? // Since allocation is serial, arrival order = disk order. @@ -2357,9 +2358,20 @@ ::asio::awaitable utxo_build_task( // on the next start the built height is what says how far the UTXO set // reaches. A set that moved without its height would be rebuilt over on // resume, applying deltas that are already in it. + // BOTH heights, and the same value (#653). `last_block_height` is the + // CONNECTED tip — what publish_chain_view builds state at, and what + // disconnect_block refuses to rewind past — so the batch that just + // connected these blocks is the only thing entitled to move it. It was + // published empty here and written instead by the storage task with the + // height of what had merely been DOWNLOADED, which is how a stopped node + // ended up claiming a connected tip 952 blocks beyond its own UTXO set. + // + // Published after step 9's barrier and inside the same transaction as the + // record, so the marker cannot outrun the data it describes: either both + // heights and the cleared record commit, or none of them do. if (auto const published = chain.publish_transition( database::transition_heights{ - .last_block_height = std::nullopt, + .last_block_height = utxo_built_height, .utxo_built_height = utxo_built_height}); published != database::result_code::success) { spdlog::critical("[utxo_build] Could not publish batch {}-{} at built height {}: the " diff --git a/src/node/test/connected_tip_remainder.cpp b/src/node/test/connected_tip_remainder.cpp new file mode 100644 index 00000000..d6f7d3c8 --- /dev/null +++ b/src/node/test/connected_tip_remainder.cpp @@ -0,0 +1,202 @@ +// Copyright (c) 2016-present Knuth Project developers. +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include + +#include "sync_harness.hpp" + +using namespace kth; +using namespace kth::test; + +// ============================================================================= +// The trailing remainder, in the shape that produced the mainnet livelock (#653) +// ============================================================================= +// +// A mainnet node reached 962946 of 963887 and stopped there for over an hour, +// with no error: 941 blocks downloaded and validated that never entered the UTXO +// set. The builder drains a remainder shorter than one batch only when NOT +// stale, and staleness was answered from the CONNECTED tip — the very height the +// remainder would advance. Stale kept the remainder unbuilt, unbuilt kept the +// connected tip 941 blocks back, and a connected tip six days old kept the +// answer stale. +// +// Why no test caught it: this harness mines blocks whose timestamps are recent, +// so the connected tip always looked current and the broken input never showed. +// The case below is built the other way round on purpose — a connected chain +// whose blocks are genuinely old, under a header tip that is current — which is +// what a node partway through a real sync actually looks like. +// +// Recency now comes from the validated header tip, which nothing the builder +// does can move. + +namespace { + +constexpr uint32_t block_spacing = 600; + +// A run of blocks whose timestamps are DAYS old, then a run that is current. +// The old ones stand for what a sync has connected so far; the recent ones for +// the headers the node already has from the network. +struct aged_chain { + std::vector old_blocks; + std::vector recent_blocks; +}; + +aged_chain make_aged_chain(domain::chain::block const& genesis, + uint32_t old_len, uint32_t recent_len) { + aged_chain out; + auto prev = genesis.hash(); + uint32_t height = 1; + + // Six days back, which is what 941 blocks of spacing comes to — comfortably + // past the 24 hour staleness limit. + auto const old_base = uint32_t(zulu_time()) - (6 * 24 * 3600); + for (uint32_t i = 0; i < old_len; ++i) { + out.old_blocks.push_back( + mine_block(prev, height, old_base + i * block_spacing, 0, {}, 0)); + prev = out.old_blocks.back().hash(); + ++height; + } + + auto const recent_base = uint32_t(zulu_time()) - (recent_len + 2) * block_spacing; + for (uint32_t i = 0; i < recent_len; ++i) { + out.recent_blocks.push_back( + mine_block(prev, height, recent_base + i * block_spacing, 0, {}, 0)); + prev = out.recent_blocks.back().hash(); + ++height; + } + return out; +} + +} // namespace + +TEST_CASE("a remainder shorter than one batch is built while the node runs", + "[node][connected_tip][remainder]") { + // THE DISCRIMINATING CONTROL. The connected chain is six days old and the + // header tip is current — the mainnet shape — and the remainder is far + // shorter than the 1000-block batch, so it is drained only if the node is + // judged current. No stop, no restart: the run that connects them is the run + // that must finish them. + chain_fixture fixture("remainder_live"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + auto const chain_blocks = make_aged_chain(domain::chain::block::genesis_regtest(), 4, 3); + + // Every header the node has, old and recent alike: this is the header sync + // having run ahead of the build, which is what puts a current tip above an + // old connected chain. + std::vector all; + all.insert(all.end(), chain_blocks.old_blocks.begin(), chain_blocks.old_blocks.end()); + all.insert(all.end(), chain_blocks.recent_blocks.begin(), chain_blocks.recent_blocks.end()); + REQUIRE(fixture.organizer().add_headers(headers_of(all)).headers_added == all.size()); + persist_headers(fixture, all, 1); + + // The header tip is current, so the node is NOT behind the network — even + // though nothing has been connected yet and the connected marker is at 0. + CHECK_FALSE(chain.is_stale()); + + // Connect only the old run: 4 blocks, a remainder far below batch_size. + connect_bodies(fixture, chain_blocks.old_blocks, 1); + + auto const built = chain.get_utxo_built_height(); + REQUIRE(built); + CHECK(*built == 4u); // drained, rather than parked one batch short + + // And both markers describe the same block, which is the invariant the + // publish is responsible for. + auto const heights = chain.get_last_heights(); + REQUIRE(heights); + CHECK(heights->block == 4u); + CHECK(heights->block == *built); +} + +TEST_CASE("staleness does not come from the height the builder is trying to reach", + "[node][connected_tip][remainder]") { + // The negation of the case above, stated as a property rather than a run: + // with a current header tip, the connected marker cannot make the node stale + // no matter how far behind it is. Reading staleness from the connected tip + // is what closes the loop, so this is the assertion that would go red if it + // ever went back to doing that. + chain_fixture fixture("remainder_not_connected_tip"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + auto const chain_blocks = make_aged_chain(domain::chain::block::genesis_regtest(), 4, 3); + std::vector all; + all.insert(all.end(), chain_blocks.old_blocks.begin(), chain_blocks.old_blocks.end()); + all.insert(all.end(), chain_blocks.recent_blocks.begin(), chain_blocks.recent_blocks.end()); + REQUIRE(fixture.organizer().add_headers(headers_of(all)).headers_added == all.size()); + persist_headers(fixture, all, 1); + + // The connected marker pinned at genesis — as far behind as it can be. + REQUIRE(chain.set_last_block_height(0) == database::result_code::success); + CHECK_FALSE(chain.is_stale()); + + // And pinned at the old connected tip, whose header is six days old: still + // not stale, because that is not where the answer comes from. + REQUIRE(chain.set_last_block_height(4) == database::result_code::success); + CHECK_FALSE(chain.is_stale()); +} + +TEST_CASE("storing blocks does not advance the connected tip", + "[node][connected_tip][remainder]") { + // Storing makes a block downloadable, not connected: the bytes are in a + // stdio buffer, the index entry is in memory, and no barrier has run. The + // storage task used to write that height into the connected marker — and + // only on the way out — which is how a stopped node came to claim a + // connected tip 952 blocks beyond its own UTXO set. + chain_fixture fixture("remainder_store_only"); + REQUIRE(fixture.created()); + REQUIRE(fixture.start()); + auto& chain = fixture.chain(); + + auto const chain_blocks = make_aged_chain(domain::chain::block::genesis_regtest(), 4, 0); + REQUIRE(fixture.organizer().add_headers(headers_of(chain_blocks.old_blocks)).headers_added + == chain_blocks.old_blocks.size()); + persist_headers(fixture, chain_blocks.old_blocks, 1); + + auto const before = chain.get_last_heights(); + REQUIRE(before); + + // The STORAGE TASK, on its own — no build task behind it. Driving + // store_chunk() directly would only test the API; what has to be pinned is + // that the task which stores does not publish a connected tip. + std::vector> light; + for (auto const& blk : chain_blocks.old_blocks) { + light.push_back(to_light(blk)); + } + + ::asio::io_context ctx; + block_storage_input_channel input(ctx.get_executor(), 16); + chunk_validated_channel output(ctx.get_executor(), 256); + std::atomic contiguous{1}; + + ::asio::co_spawn(ctx, + block_storage_task(chain, input, output, 1, fixture.organizer(), &contiguous), + ::asio::detached); + + REQUIRE(input.try_send(std::error_code{}, downloaded_chunk{ + .start_height = 1, + .chunk_id = 0, + .blocks = std::move(light), + .source_peer = nullptr, + .generation = chain.headers().generation() + })); + REQUIRE(input.try_send(std::error_code{}, stop_request{})); + ctx.run_for(std::chrono::seconds(60)); + while (output.try_receive([](std::error_code, chunk_validated) {})) {} + + // The blocks really were stored, so the assertion below is about a task that + // did its work rather than one that did nothing. + CHECK(contiguous.load() > 1u); + + auto const after = chain.get_last_heights(); + REQUIRE(after); + CHECK(after->block == before->block); // unmoved by storage alone +}