Repository navigation
Conversation
… sign_data The old sentence claimed an agent key "cannot pass passwordless site auth or sign_data". There is no `sign_data` anywhere in the chain (the name occurred only in that line), and the claim mixed two layers: consensus sees operations only, while a `domain:auth:account:regular:time:nonce` signature is off-chain. The verifying libraries (VIZ\Auth in viz-php-lib, viz.auth in viz-js-lib) accept an agent for such a signature when the agent row carries the site's addon - there the addon, not the operation list, is the grant (decision q#2152=A). Both layers stated in en and @l10n/ru.
… silently The label lived in an `account_name_type` (32 bytes) whose constructor TRUNCATES: a string longer than 32 bytes was memcpy'd down to its first 32, with no error anywhere. validate() only checked emptiness and the character set, so the declared "1..32" bound was never enforced. The consequence is not cosmetic. Two labels sharing the first 32 bytes collapse into the same (principal, agent_name) key of `by_permission_account`, so the second grant REPLACES the first row - rotating its key, operation list and expiration - while the caller believes it added an agent. The label is an agent's identity, so silently renaming one is the wrong failure mode. Fix: the op carries the label as a plain string and validate() refuses anything longer than CHAIN_MAX_ACCOUNT_NAME_LENGTH (32) while the raw length is still visible. The evaluator compares through the string form too, so the name check cannot pass through the truncating conversion. Wire/JSON compatible: a fixed_string packs and reflects exactly as a string does (proved: bytes and JSON form identical for one and the same label), so no transaction changes shape and nothing has to be re-signed or re-snapshotted. Test: pm_agent_access pins the boundary - 33 bytes refused, 32 bytes accepted. Reverting only the fix (test kept) makes the new check fail, so it is not a test that cannot fail.
The block-application path resolved required authorities through
get_account(), which throws a generic chainbase out_of_range ("unknown
key") when a name is absent. That type lands in the generic fc::exception
branch of the p2p / chain / validator handlers, which merely rejects the
block and leaves the node sitting behind head — exactly the 14.10 incident.
Discriminate the two cases the bare exception conflates. This path already
holds the strong write lock (push_block / push_transaction take it for the
whole application), so a miss cannot be a racing writer, and a nested
with_strong_read_lock is not an option: read_write_mutex is a non-recursive
boost::shared_mutex, so a second lock from this thread would deadlock.
The discriminator is the index that does not break: by_id is a flat vector,
by_name is the red-black tree. An account reachable only by id means the
name index lost a live entry — log the diagnostics and throw
shared_memory_corruption_exception, which the existing handlers turn into
attempt_auto_recovery(). An account absent from both indexes is a block
naming a nonexistent account: ordinary rejection, behaviour unchanged.
| @@ -6065,6 +6075,39 @@ namespace graphene { namespace chain { | |||
| const validator_schedule_object &consensus = get_validator_schedule_object(); | |||
There was a problem hiding this comment.
This discriminates corruption only for accounts that are required signers of the transaction (required comes from trx.get_required_authorities(...)). database::get_account() is called directly, with no such guard, in ~115 other places across the evaluators — including the most common one of all:
// transfer_evaluator::do_apply, chain_evaluator.cpp:887
const auto &from_account = _db.get_account(o.from); // signer -> covered by this check
const auto &to_account = _db.get_account(o.to); // not a signer -> NOT coveredo.to never appears in required (receiving a transfer needs no signature), so if the index damage happens to make the receiver's name unreachable rather than the sender's, get_account(o.to) still throws the bare fc::assert_exception — not shared_memory_corruption_exception — and the node falls right back into the stuck-behind-head failure this PR is meant to fix. Same hole in escrow_transfer_evaluator (o.to, o.agent at lines 720), benefactor accounts in award_evaluator/fixed_award_evaluator, invite_evaluator's receiver, etc.
The PR description doesn't say which account died in get_account() during the 14.10 incident (block 84284542). If it wasn't a required signer, this patch wouldn't have caught the incident it's written to fix. Pushing the by_id discrimination down into get_account()/find_account itself (so every one of the ~115 call sites inherits it for free) would close the gap without duplicating this block at every caller.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 4446e11: the by_id discrimination now lives in database::get_account() itself, so all evaluator call sites (transfer to, escrow to/agent, award beneficiaries, invite receiver, …) inherit it. It is gated by a thread-local apply depth raised in _apply_block/_apply_transaction, so only the write-lock holder can turn a by_name miss into shared_memory_corruption_exception; lockless readers on other threads (API, block-production hints) keep the old behaviour and can't trigger a false recovery. find_account() is unchanged (existence checks stay O(log n)); the O(accounts) scan runs only on a miss, which already rejects the op. The duplicated block in _apply_transaction is removed. #180 (P2P routing) is cherry-picked on top as 8d723cf.
Review of #179 (chiliec): the by_id discrimination only covered required signers, while ~115 evaluator call sites use get_account() directly (e.g. the transfer receiver). Move the check into database::get_account(), gated by a thread-local apply depth raised in _apply_block and _apply_transaction, so only the write-lock holder can turn a miss into shared_memory_corruption_exception; lockless readers on other threads keep the old behaviour.
3 participants
Commits in this PR
pmis the working branch, so this pull request carries the whole batch (same shape as #177, #175, #171):f2517168by_nameas shared-memory corruptiond7e2ad1eagent_nameinstead of truncating it silently9e30eb9fsign_dataTo review only the corruption fix: commit
f2517168.Problem
A node that loses a live account from the
by_nameindex does not recover: it stops applyingblocks and sits behind head until an operator intervenes. That is what happened in the 14.10
incident — block 84284542 died in
get_account()with a bareunknown key(out_of_rangefrom chainbase), and
_push_block/ the p2p layer rejected the block exactly as if it weremerely invalid.
The node already has the machinery for the opposite behaviour:
shared_memory_corruption_exceptionis caught in four places (
chain::plugin::accept_block/accept_transaction,p2p_plugin::accept_block,validator) and funnels intoattempt_auto_recovery(), which pausesP2P, wipes the database and reloads the newest snapshot (under a 3-attempt / 5-minute guard).
Those handlers cannot fire for this failure, because the block-application path never throws
that type.
Change
In
database::_apply_transaction, when a required authority is missing fromby_name,discriminate the two cases the bare exception conflates:
by_idbut not by name — the name index lost a live entry: log thediagnostics (
head,trx,expiration,account_index_size) and throwshared_memory_corruption_exception, so the existing handlers start recovery.to
get_account()and reject exactly as before.by_idis an independent ordered index (also a tree, not a flat vector). A live objectreachable by id but not by name proves an index inconsistency; this does not assume that
the id index is immune to corruption.
Locking
This path already runs under an exclusive write lock (
push_block/push_transactionhold it forthe whole application), so a missing name cannot be a racing writer. A nested
with_strong_read_lockis not available here either:read_write_mutexis a non-recursiveboost::shared_mutex, so a second lock from the same thread would deadlock. The re-read istherefore done under the lock already held, which is the strongest available form of the check.
Cost
The
by_idscan runs only after a name lookup has already failed (a block naming a nonexistentaccount — tens per hour on mainnet) and walks ~14k entries.
Deployment
Measured on prod (
docker top vizd): the running command line is--auto-recover-from-snapshotis not on it, and it does not need to be: the option isbool_switch()->default_value(true)(plugins/chain/plugin.cpp:411, added by88bc7768, inmastersince 28.04.2026 — long before the deployed 01.10 image was built). So the live binaryalready has self-recovery enabled; the only thing missing is the exception type that reaches it,
which is what this change supplies. Container
RestartPolicy=nois unchanged and remains correct:attempt_auto_recovery()either recovers or callsappbase::app().quit()deliberately.Verification
vizd:pmimage) — success;Tests— success (both onf2517168).-fsyntax-onlyondatabase.cpp— clean.restores the original keys before teardown, and checks that a live name-index miss throws the
typed corruption exception while an account absent from both indexes remains an ordinary rejection.
Audit follow-up
Preserve
shared_memory_corruption_exceptionacross the four block-application catch sitesthat otherwise consume it as an ordinary failure: fork application, its schedule-relaxed retry,
_push_nextcascade application, and pending transaction selection during block generation.Each now rethrows before the generic
fc::exceptionhandler, so chain/P2P/validator recoverycan receive the original type. Ordinary invalid-block and invalid-transaction handling is unchanged.
The agent-name regression also checks legacy byte parity at lengths 1 and 32, plus
wire round-trip preservation and rejection of a 33-byte name.
Pending/popped transaction restoration also propagated neither exception: it lived in a
noexcept destructor and swallowed per-transaction failures. Restoration is now an explicit
call after callback completion (including ordinary callback failure), with typed corruption
rethrows. An already detected corruption skips reapplication. Regression coverage exercises
both pending and popped restoration and preservation of an ordinary callback exception.
Local audit verification (0441489)
equivocation seed_deadbeef scenario: passed.
harness ran with ASan/UBSan and leak detection disabled. Full release validation is the new CI run.