Prevent transaction replay and stop reorgs from destroying transactions

Two gaps in the account model, found while working out how many confirmations
are actually needed for a payment to be safe.

== A reorg silently destroyed transactions ==

chain.c removed transactions from the mempool when they were mined but never
put them back. TxMempool_Insert had exactly two callers -- the `send` command
and inbound broadcasts -- so a transaction that existed only in an orphaned
block was gone from both the chain and the pool, and would never be mined
unless its sender happened to rebroadcast.

That makes the usual reassurance about accidental orphans ("your transaction
just lands in the next block") false for this node. Chain_RollbackToHeightLocked
now returns the discarded blocks' non-coinbase transactions to the mempool
before anything is freed, going through the existing Chain_BorrowBlockTransactions
disk fallback because those blocks are usually header-only by then.

Coinbases are deliberately not restored: they are bound to a specific height and
reward and are invalid anywhere else.

A transaction that also appears in the replacing branch needs no special
handling. Chain_ReplaceBranch rolls back and applies under a single lock
acquisition, and Chain_AddBlockLocked removes every applied transaction from the
pool again -- so it is re-inserted and removed moments later, with no window in
which a miner could pick up a copy of something already back in the chain. That
ordering is what makes this safe in an account model, where re-mining a
transaction would debit the sender twice, so it is spelled out at the site.

== Any historical transaction could be replayed ==

Nothing rejected a transaction whose hash was already in the chain.
Block_AllTransactionsValid checks signatures and that there is exactly one
coinbase; it never looked for repeats, and the mempool's hash-keyed dedup is
mempool-only. So anyone could take a mined, publicly visible signed transaction,
rebroadcast it, and have it mined again -- debiting the sender a second time.
UTXO chains get this for free because the spent inputs no longer exist; an
account model has nothing to stop it.

balance_sheet_entry_t gains lastTxTimestamp, and a non-coinbase transaction is
now valid only if its timestamp is strictly greater than its sender's last
included one. Transaction timestamps are unix MILLISECONDS (the comment in
transaction.h claimed seconds and was stale), so two genuinely distinct
transactions never collide and an exact collision means a byte-identical copy --
precisely what must be refused.

Coinbase is exempt: a block may hold only one, its amount is pinned to the
height, and including someone else's would only donate the miner's own reward.

Three deliberate choices:

  * Enforced in Chain_AddBlockLocked, BEFORE the push. That function is the only
    insertion point into the chain besides the header-only disk load, so mining,
    broadcast, windowed sync, orphan attach and reorg apply are all covered by
    one check rather than four copies. Putting it in the ledger pass instead
    would be too late -- that runs after the block is in the chain and can only
    return false, leaving an invalid block behind.
  * Extracted as Chain_BlockRespectsSenderOrdering rather than left inline,
    so the multi-sender case can be tested directly. Senders are strictly
    independent: one account's timestamps say nothing about another's, and
    folding them together would reject ordinary blocks outright.
  * DebitAddress takes the timestamp and advances the guard in the same call, so
    a spend cannot happen without the guard moving. They cannot drift apart.

Rebuilt for free on reorg: the rollback already destroys and replays the whole
balance sheet, so setting lastTxTimestamp in that same loop means there is no
separate invalidation path to get wrong.

== The miner's fee sort broke the new rule ==

CompareTransactionPriority orders by fee descending, so a sender's later,
higher-fee transaction could sort ahead of their earlier, lower-fee one -- and
Chain_AddBlockLocked walks a block in order, so the node would have built blocks
its own rule rejects.

This cannot be folded into the comparator: "higher fee first, except same sender
by time" is not a strict weak ordering (A beats B on fee, B beats C on fee, C
beats A on time) and qsort with an inconsistent comparator is undefined. Instead
the priority sort is followed by a permutation restricted to each sender's own
slots, so fee-based slot allocation survives untouched and only the order within
one sender's slots changes.

== Mempool timestamp policy (local policy, NOT consensus) ==

Separate concern: keeping junk out of the pool. TX_MAX_FUTURE_DRIFT_MS (2h) and
TX_EXPIRY_MS (4 days, chosen to roughly match what DIFFICULTY_ADJUSTMENT_INTERVAL
spans but in milliseconds so it does not drift with block time), both in
constants.h. Gated at both admission sites via TxMempool_PolicyAccepts, with
TxMempool_PruneExpired on the 1Hz maintenance tick.

Blocks are never rejected for either bound, so a node with a skewed clock cannot
fork itself off the network over an admission rule.

Both bounds are measured against the node's own clock, NOT against the chain
tip. Measuring "future" against the last block assumes blocks keep arriving: on a
quiet chain the tip can be hours old, and an honest transaction created right now
would look hours ahead of it and be refused -- making sending impossible exactly
when the chain is idle.

A too-old timestamp needs no rule here; the replay guard already refuses anything
at or below a sender's last.

== Verification ==

New unit suite, 21 assertions against the real objects, all passing. The ones
that matter:

  multi-sender independence
    alice(6000) and bob(101) in one block          -> accepted
    reversed order                                  -> accepted
    8 senders sharing one timestamp                 -> accepted
  replay
    equal to sender's last (carbon copy)            -> rejected
    older than sender's last                        -> rejected
    same transaction twice in one block             -> rejected
  ordering within a block
    same sender increasing                          -> accepted
    same sender decreasing                          -> rejected
  policy window
    inside/outside both bounds, and pruning         -> as specified

Without the sender comparison in the guard, the first three would all fail --
that is the case worth guarding against, because the check reads as if it folds
all senders together.

penalty_test and dag_test suites still pass. AddressSanitizer reported zero
errors on both nodes across the reorg path, which is the signal that matters
given the rollback now does more work.

== Not yet verified ==

End-to-end replay rejection and reorg-restores-mempool against live nodes; both
need two funded wallets, so they are a separate setup. The fork regression
(forksync 5 60) was still climbing toward its penalty threshold when this was
written -- branchLead 40 of the required 42, behaving correctly but not yet
adopted.

== Note ==

balance_sheet_entry_t is written raw to disk, so balance_sheet.data gains a
field and old files will not load. wipechain before running.

Related gap, pre-existing and NOT addressed: the disk load restores headers only
and Chain_RecomputeRuntimeState does not rebuild balances, so lastTxTimestamp
survives a restart purely because it is persisted in balance_sheet.data -- and
that file has no height marker to detect it being stale against the chain. A node
with a missing or out-of-date sheet silently resets every account's guard to 0
and allows one replay per account. Balances are already wrong in that situation
today, but this change turns it from an accounting bug into a security one.
Recording the chain height alongside the sheet and refusing to start on a
mismatch is the natural follow-up.
This commit is contained in:
2026-08-03 16:50:16 +02:00
parent 393d26dfcb
commit 10d5d71a9f
9 changed files with 331 additions and 5 deletions
+14
View File
@@ -16,6 +16,20 @@
typedef struct {
uint8_t address[32]; // For now just the SHA-256 of the public key; allows representation in different encodings (base58, bech32, etc) without changing the underlying data structure
uint256_t balance;
/**
* Timestamp (unix ms) of the most recent transaction this address SENT that is in the chain.
*
* Replay protection. Without it any historical transaction could be rebroadcast and mined a
* second time, debiting the sender again -- with UTXOs the spent inputs make that impossible,
* but an account model has nothing to stop it. A non-coinbase transaction is only valid if its
* timestamp is strictly greater than this, so a byte-identical replay (same timestamp, same
* hash) can never be included twice. Enforced in Chain_AddBlockLocked; see the note there.
*
* Rebuilt for free by the rollback's balance-sheet replay, so a reorg cannot leave it stale.
* Persisted with the rest of the entry -- note the file has no height marker, so a balance
* sheet that is out of sync with the chain silently resets this to 0 for every account.
**/
uint64_t lastTxTimestamp;
// TODO: Additional things
} balance_sheet_entry_t;
+14
View File
@@ -90,6 +90,20 @@ bool Chain_IsInitialBlockDownload(blockchain_t* chain);
// want to report it.
uint64_t Chain_ReorgPenaltyForDepth(uint64_t reorgDepth);
/**
* Replay guard: true if every non-coinbase transaction in `block` is newer than its own sender's
* last included transaction, and newer than that same sender's earlier transactions in this block.
*
* Reads the balance sheet's per-account `lastTxTimestamp` (see balance_sheet.h). Senders are
* considered independently -- one account's transactions say nothing about another's ordering, so
* an ordinary block full of different senders always passes. Coinbase is exempt.
*
* Exposed rather than inlined so this can be tested directly; Chain_AddBlockLocked calls it as part
* of block validation, which is what makes it apply to mining, sync, broadcast, orphan attach and
* reorg alike.
**/
bool Chain_BlockRespectsSenderOrdering(const block_t* block);
// Recompute `currentSupply` and `currentReward` from the in-memory chain blocks.
// Returns true on success and updates runtime state globals.
bool Chain_RecomputeRuntimeState(blockchain_t* chain);
+5 -1
View File
@@ -23,7 +23,11 @@ static inline bool Address_IsCoinbase(const uint8_t address[32]) {
// 168 bytes total for v1
#pragma pack(push, 1) // Ensure no padding for consistent file storage
typedef struct {
uint64_t timestamp; // Unix timestamp in seconds - not enforced, but used for uniqueness when everything else is the same.
uint64_t timestamp; // Unix timestamp in MILLISECONDS (get_current_time_ms). Two of a sender's
// transactions must have strictly increasing timestamps -- see
// lastTxTimestamp in balance_sheet.h. Millisecond resolution is what makes
// an exact collision mean 'byte-identical replay' rather than 'two real
// transactions that happened to coincide'.
uint64_t fee; // Rewarded to the miner; can be zero, but the miner may choose to ignore transactions with very low fees
uint64_t amount1;
uint64_t amount2;
+19
View File
@@ -91,6 +91,25 @@ static const uint64_t REORG_PENALTY_REF_BLOCK_TIME = 150ULL; // reference block
// from overflow rather than to bound the penalty in any meaningful sense.
static const uint64_t REORG_PENALTY_MAX_DEPTH = 1000ULL;
/**
* Mempool transaction timestamp policy. LOCAL POLICY, NOT CONSENSUS.
*
* These govern what this node is willing to hold and relay; a block containing a transaction that
* violates either is still accepted. That separation is deliberate -- a node with a skewed clock
* must not be able to fork itself off the network over an admission rule.
*
* A too-OLD timestamp needs no rule here: the per-account replay guard (see balance_sheet.h) already
* refuses anything at or below a sender's last included transaction.
**/
// Refuse to admit a transaction dated further ahead than this of OUR OWN CLOCK. Measured against
// the clock and not against the chain tip on purpose: on a quiet chain the tip can be hours old, and
// judging "future" against it would refuse honest transactions exactly when blocks are sparse.
static const uint64_t TX_MAX_FUTURE_DRIFT_MS = 2ULL * 60ULL * 60ULL * 1000ULL; // 2 hours
// Drop transactions older than this from the mempool, so it is not inflated by junk that will never
// be mined. Roughly the ~4 days DIFFICULTY_ADJUSTMENT_INTERVAL spans, but expressed in milliseconds
// so it does not drift if the block time changes.
static const uint64_t TX_EXPIRY_MS = 4ULL * 24ULL * 60ULL * 60ULL * 1000ULL; // 4 days
// Upper bound on pooled orphan blocks. Orphans are accepted before the chain-derived difficulty
// check (that lives in Chain_AddBlock, which orphans only reach on attach), so without a cap a
// peer can push blocks at an arbitrary height until the node runs out of memory.
+19
View File
@@ -17,6 +17,25 @@ bool TxMempool_Snapshot(signed_transaction_t** outTxs, size_t* outCount);
void TxMempool_Print();
// Remove a transaction from the mempool by its hash. Returns true if removed.
bool TxMempool_Remove(const uint8_t* txHash);
/**
* Admission policy: should this transaction be held and relayed?
*
* LOCAL POLICY, NOT CONSENSUS. A block containing a transaction this rejects is still accepted --
* see TX_MAX_FUTURE_DRIFT_MS / TX_EXPIRY_MS in constants.h for why the two are kept apart.
*
* Both bounds are measured against the node's own clock, NOT against the chain tip's timestamp.
* Measuring "future" against the last block assumes blocks keep arriving: on a quiet chain the tip
* can be hours old, and an honest transaction created right now would look hours ahead of it and be
* refused. Sending would become impossible exactly when the chain is idle.
*
* Deliberately NOT applied when a rollback returns transactions to the pool: those were already in
* the chain, so they are legitimate by definition and must not be dropped for looking old.
**/
bool TxMempool_PolicyAccepts(const signed_transaction_t* tx, uint64_t nowMs);
// Drop transactions older than TX_EXPIRY_MS. Returns how many were removed.
size_t TxMempool_PruneExpired(uint64_t nowMs);
void TxMempool_Destroy();
#endif