From 10d5d71a9f745b4a6244e252c9268c9bd47f1e73 Mon Sep 17 00:00:00 2001 From: DcruBro Date: Mon, 3 Aug 2026 16:50:16 +0200 Subject: [PATCH] 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. --- include/balance_sheet.h | 14 ++++ include/block/chain.h | 14 ++++ include/block/transaction.h | 6 +- include/constants.h | 19 +++++ include/txmempool.h | 19 +++++ src/block/chain.c | 146 +++++++++++++++++++++++++++++++++++- src/main.c | 54 ++++++++++++- src/nets/net_node.c | 17 ++++- src/txmempool.c | 47 ++++++++++++ 9 files changed, 331 insertions(+), 5 deletions(-) diff --git a/include/balance_sheet.h b/include/balance_sheet.h index 0b5b5be..5ad4b0d 100644 --- a/include/balance_sheet.h +++ b/include/balance_sheet.h @@ -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; diff --git a/include/block/chain.h b/include/block/chain.h index 20f2f2e..732b3e3 100644 --- a/include/block/chain.h +++ b/include/block/chain.h @@ -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); diff --git a/include/block/transaction.h b/include/block/transaction.h index 7ebeeac..b36165d 100644 --- a/include/block/transaction.h +++ b/include/block/transaction.h @@ -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; diff --git a/include/constants.h b/include/constants.h index 018078a..b345d5e 100644 --- a/include/constants.h +++ b/include/constants.h @@ -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. diff --git a/include/txmempool.h b/include/txmempool.h index 18071ce..2e003c0 100644 --- a/include/txmempool.h +++ b/include/txmempool.h @@ -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 diff --git a/src/block/chain.c b/src/block/chain.c index 7c32e34..ab6898c 100644 --- a/src/block/chain.c +++ b/src/block/chain.c @@ -83,7 +83,15 @@ static bool CreditAddress(const uint8_t address[32], uint64_t amount) { return BalanceSheet_Insert(entry) >= 0; } -static bool DebitAddress(const uint8_t address[32], const uint256_t* amount) { +/** + * Debit a sender and advance its replay guard in one step. + * + * `txTimestamp` is the timestamp of the transaction doing the spending; it becomes the account's + * `lastTxTimestamp`, which is what makes a byte-identical replay of that transaction invalid later. + * Taking it here rather than updating the entry separately means the debit and the guard cannot + * drift apart -- every path that spends from an account necessarily advances it. +**/ +static bool DebitAddress(const uint8_t address[32], const uint256_t* amount, uint64_t txTimestamp) { if (!address || !amount) { return false; } @@ -101,6 +109,10 @@ static bool DebitAddress(const uint8_t address[32], const uint256_t* amount) { return false; } + if (txTimestamp > entry.lastTxTimestamp) { + entry.lastTxTimestamp = txTimestamp; + } + return BalanceSheet_Insert(entry) >= 0; } @@ -424,6 +436,31 @@ static bool Chain_AddBlockLocked(blockchain_t* chain, block_t* block) { free(spendableTxs); + /** + * Replay guard: each sender's transaction timestamps must strictly increase. + * + * Without this, any transaction already in the chain could be rebroadcast and mined again, + * debiting the sender a second time. UTXO chains get this for free because the inputs no + * longer exist; an account model has nothing to stop it. Timestamps are unix milliseconds, + * so two genuinely distinct transactions never collide, and an exact collision means a + * byte-identical copy -- precisely what must be refused. + * + * Checked HERE, with the rest of block validation and before the push, rather than in the + * ledger pass below: that pass runs after the block is already in the chain and can only + * return false, leaving an invalid block behind. + * + * Placement in Chain_AddBlockLocked is what makes it apply everywhere -- mining, broadcast, + * windowed sync, orphan attach and reorg apply all funnel through here, and it is the only + * insertion point into the chain besides the header-only disk load. Do not add a second + * copy on the receive path where the symptom happens to be visible. + **/ + if (!Chain_BlockRespectsSenderOrdering(block)) { + printf("Chain_AddBlock: validation failed: blockIndex=%zu contains a transaction that is " + "not newer than its sender's last -- replay or out-of-order\n", expectedIndex); + ok = false; + break; + } + // Push the block only after validation succeeds. block_t* blk = (block_t*)DynArr_push_back(chain->blocks, block); if (!blk) { ok = false; break; } @@ -462,7 +499,8 @@ static bool Chain_AddBlockLocked(blockchain_t* chain, block_t* block) { if (!Address_IsCoinbase(tx->transaction.senderAddress)) { uint256_t spend; - if (!BuildSpendAmount(tx, &spend) || !DebitAddress(tx->transaction.senderAddress, &spend)) { + if (!BuildSpendAmount(tx, &spend) || + !DebitAddress(tx->transaction.senderAddress, &spend, tx->transaction.timestamp)) { fprintf(stderr, "Error: Failed to debit sender balance during block addition. Bailing!\n"); ok = false; break; } @@ -631,6 +669,52 @@ static bool Chain_RollbackToHeightLocked(blockchain_t* chain, size_t height) { return true; // nothing to do } + /** + * Return the discarded blocks' transactions to the mempool before anything is freed. + * + * Without this a reorg silently destroys them: they were removed from the mempool when mined + * (see Chain_AddBlockLocked) and nothing ever put them back, 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. An accidental orphan should cost a transaction one + * block of delay, not its existence. + * + * 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 branch replacing this one needs no special handling: + * Chain_ReplaceBranch rolls back and then applies under a single lock acquisition, and + * Chain_AddBlockLocked removes every applied transaction from the pool again. So it is + * re-inserted here and removed again 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. + * + * Blocks are usually header-only by now (anything saved or loaded has transactions == NULL), so + * this goes through the same borrow/disk-fallback pair the balance-sheet replay below uses. + **/ + for (size_t i = cur; i > height; --i) { + const size_t idx = i - 1; + + block_t* source = NULL; + bool loadedFromDisk = false; + if (!Chain_BorrowBlockTransactions(chain, idx, &source, &loadedFromDisk)) { + // Nothing recoverable for this block; the rollback itself must still proceed. + continue; + } + + if (source && source->transactions) { + const size_t txCount = DynArr_size(source->transactions); + for (size_t t = 0; t < txCount; ++t) { + signed_transaction_t* tx = (signed_transaction_t*)DynArr_at(source->transactions, t); + if (!tx || Address_IsCoinbase(tx->transaction.senderAddress)) { + continue; + } + (void)TxMempool_Insert(*tx); + } + } + + Chain_ReturnBlockTransactions(source, loadedFromDisk); + } + // Remove blocks above height for (size_t i = cur; i > height; --i) { size_t idx = i - 1; @@ -702,6 +786,12 @@ static bool Chain_RollbackToHeightLocked(blockchain_t* chain, size_t height) { senderEntry.balance = uint256_from_u64(0); } (void)uint256_subtract(&senderEntry.balance, &spend); + // Rebuild the replay guard in the same pass that rebuilds the balance. Because this + // replay is what a rollback already does, the guard needs no invalidation logic of + // its own -- after any reorg it reflects exactly the blocks that remain. + if (tx->transaction.timestamp > senderEntry.lastTxTimestamp) { + senderEntry.lastTxTimestamp = tx->transaction.timestamp; + } (void)BalanceSheet_Insert(senderEntry); // Credit recipient1 @@ -872,6 +962,58 @@ static void Chain_FreeBlockArray(block_t** blocks, size_t count) { free(blocks); } +bool Chain_BlockRespectsSenderOrdering(const block_t* block) { + if (!block || !block->transactions) { + return false; + } + + const size_t txCount = DynArr_size(block->transactions); + + for (size_t i = 0; i < txCount; ++i) { + const signed_transaction_t* tx = (const signed_transaction_t*)DynArr_at(block->transactions, i); + if (!tx || Address_IsCoinbase(tx->transaction.senderAddress)) { + continue; // coinbase is exempt -- see balance_sheet.h + } + + // Baseline is THIS sender's own last included transaction. + uint64_t lastSeen = 0; + balance_sheet_entry_t senderEntry; + if (BalanceSheet_Lookup((uint8_t*)tx->transaction.senderAddress, &senderEntry)) { + lastSeen = senderEntry.lastTxTimestamp; + } + + /** + * Fold in earlier transactions from THE SAME SENDER in this same block, which the sheet + * does not know about yet: the ledger applies a block in order, so they must be ordered. + * + * The sender comparison is load-bearing. Without it this would absorb every other sender's + * timestamps and reject ordinary multi-sender blocks -- Alice's transaction would be judged + * against Bob's. Senders are strictly independent here. + * + * Quadratic in the worst case (a whole block from one sender). Fine while blocks are small; + * worth a per-sender map if they grow. + **/ + for (size_t j = 0; j < i; ++j) { + const signed_transaction_t* prev = (const signed_transaction_t*)DynArr_at(block->transactions, j); + if (!prev || Address_IsCoinbase(prev->transaction.senderAddress)) { + continue; + } + if (memcmp(prev->transaction.senderAddress, tx->transaction.senderAddress, 32) != 0) { + continue; // different sender -- says nothing about this one's ordering + } + if (prev->transaction.timestamp > lastSeen) { + lastSeen = prev->transaction.timestamp; + } + } + + if (tx->transaction.timestamp <= lastSeen) { + return false; + } + } + + return true; +} + uint64_t Chain_ReorgPenaltyForDepth(uint64_t reorgDepth) { return FetchScheduler_ComputeReorgPenaltyBlocks(reorgDepth); } diff --git a/src/main.c b/src/main.c index e7531c9..a70bc7c 100644 --- a/src/main.c +++ b/src/main.c @@ -202,6 +202,52 @@ static void AddCoinbaseTransaction(block_t* block, const uint8_t minerAddress[32 Block_AddTransaction(block, &coinbaseTx); } +/** + * Put each sender's transactions into timestamp order, without disturbing anyone else's position. + * + * Chain_AddBlockLocked requires a sender's timestamps to strictly increase and walks a block in + * order, so a sender's later, higher-fee transaction sorted ahead of their earlier one would make + * the block we just built invalid by our own rule. + * + * This cannot be folded into CompareTransactionPriority. "Higher fee first, except same sender goes + * 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 behaviour. So sort by priority first, then + * permute each sender's transactions among the slots they already occupy: fee-based slot allocation + * survives untouched and only the order within one sender's own slots changes. + * + * Selection sort restricted to each sender's slots. Quadratic in the worst case (a whole block from + * one sender); fine while blocks are small. +**/ +static void OrderSameSenderByTimestamp(signed_transaction_t* txs, size_t count) { + if (!txs || count < 2) { + return; + } + + for (size_t i = 0; i < count; ++i) { + if (Address_IsCoinbase(txs[i].transaction.senderAddress)) { + continue; + } + + size_t earliest = i; + for (size_t j = i + 1; j < count; ++j) { + // Only ever compare against the SAME sender, so other senders keep their fee ranking. + if (memcmp(txs[j].transaction.senderAddress, txs[i].transaction.senderAddress, 32) != 0) { + continue; + } + if (txs[j].transaction.timestamp < txs[earliest].transaction.timestamp) { + earliest = j; + } + } + + if (earliest != i) { + // Both slots belong to the same sender, so this swap cannot reorder anyone else. + signed_transaction_t tmp = txs[i]; + txs[i] = txs[earliest]; + txs[earliest] = tmp; + } + } +} + static int CompareTransactionPriority(const void* lhs, const void* rhs) { const signed_transaction_t* left = (const signed_transaction_t*)lhs; const signed_transaction_t* right = (const signed_transaction_t*)rhs; @@ -241,6 +287,7 @@ static bool BuildSpendableMempoolSelection( if (snapshot && snapshotCount > 1) { qsort(snapshot, snapshotCount, sizeof(signed_transaction_t), CompareTransactionPriority); + OrderSameSenderByTimestamp(snapshot, snapshotCount); } signed_transaction_t* acceptedTxs = NULL; @@ -1162,7 +1209,12 @@ int main(int argc, char* argv[]) { printf("send committed in mined block\n"); */ - // Insert into txmempool + // Insert into txmempool, subject to the same admission policy peers apply, so we do + // not broadcast something the rest of the network will decline to hold. + if (!TxMempool_PolicyAccepts(&spendTx, get_current_time_ms())) { + printf("transaction timestamp is outside the accepted window, not sending\n"); + continue; + } if (TxMempool_Insert(spendTx) < 0) { printf("failed to add transaction to mempool, transaction rejected\n"); continue; diff --git a/src/nets/net_node.c b/src/nets/net_node.c index a102cde..34b576b 100644 --- a/src/nets/net_node.c +++ b/src/nets/net_node.c @@ -457,6 +457,14 @@ static void* Node_MaintenanceThread(void* arg) { BalanceSheet_SaveToFile(chainDataDir); } } + // Drop transactions too old to be worth holding, so the pool is not inflated by junk that + // will never be mined. Policy only -- a block containing one is still accepted. + { + const size_t pruned = TxMempool_PruneExpired(get_current_time_ms()); + if (pruned > 0) { + printf("Maintenance: pruned %zu expired transaction(s) from the mempool\n", pruned); + } + } // Reclaim outbound slots whose peer has disconnected so they can be reused. Node_ReapDeadOutbound(n); // Peer discovery tick: ping/query connected peers and connect to the best-ping discoveries. @@ -1235,7 +1243,14 @@ void Node_Server_OnData(tcp_connection_t* client) { return; } - // Push to mempool if it's not already present + // Push to mempool if it's not already present, subject to admission policy. + // Policy only: a block containing this transaction is still accepted even if we + // decline to hold or relay it ourselves. + if (!TxMempool_PolicyAccepts(&tx, get_current_time_ms())) { + printf("Declined transaction from node %u: timestamp outside the accepted window\n", + client ? client->connectionId : 0U); + return; + } if (!TxMempool_Lookup(txHash, &tx)) { if (TxMempool_Insert(tx) >= 0) { printf("Added transaction %s from node %u to mempool\n", txHashHex, client ? client->connectionId : 0U); diff --git a/src/txmempool.c b/src/txmempool.c index 6ac4bda..95dd105 100644 --- a/src/txmempool.c +++ b/src/txmempool.c @@ -1,4 +1,5 @@ #include +#include #include static pthread_mutex_t g_txMempoolLock; @@ -12,6 +13,52 @@ void TxMempool_Init() { g_txMempoolLockInitialized = true; } +bool TxMempool_PolicyAccepts(const signed_transaction_t* tx, uint64_t nowMs) { + if (!tx) { + return false; + } + + const uint64_t ts = tx->transaction.timestamp; + + // Dated too far in the future, measured against OUR CLOCK rather than the chain tip -- see the + // note in the header. Refusing this also limits the one real footgun in the replay guard: a + // wildly future timestamp permanently advances that account's lastTxTimestamp and locks it out + // until real time catches up. + if (ts > nowMs && (ts - nowMs) > TX_MAX_FUTURE_DRIFT_MS) { + return false; + } + + // Too old to be worth holding. Not a validity judgement -- just pool hygiene. + if (nowMs > ts && (nowMs - ts) > TX_EXPIRY_MS) { + return false; + } + + return true; +} + +size_t TxMempool_PruneExpired(uint64_t nowMs) { + if (!txMempool) { + return 0; + } + + size_t removed = 0; + + pthread_mutex_lock(&g_txMempoolLock); + for (khiter_t k = kh_begin(txMempool); k != kh_end(txMempool); ++k) { + if (!kh_exist(txMempool, k)) { + continue; + } + const uint64_t ts = kh_value(txMempool, k).transaction.timestamp; + if (nowMs > ts && (nowMs - ts) > TX_EXPIRY_MS) { + kh_del(tx_mempool_map_m, txMempool, k); + removed++; + } + } + pthread_mutex_unlock(&g_txMempoolLock); + + return removed; +} + int TxMempool_Insert(signed_transaction_t tx) { if (!txMempool) { return -1; }