dcrubro
10d5d71a9f
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.
66 lines
2.7 KiB
C
66 lines
2.7 KiB
C
#ifndef TRANSACTION_H
|
|
#define TRANSACTION_H
|
|
|
|
#include <stdint.h>
|
|
#include <stdbool.h>
|
|
#include <crypto/crypto.h>
|
|
|
|
// Special sender/recipient address marker for coinbase logic: 32 bytes of 0xFF.
|
|
static inline bool Address_IsCoinbase(const uint8_t address[32]) {
|
|
if (!address) {
|
|
return false;
|
|
}
|
|
|
|
for (size_t i = 0; i < 32; ++i) {
|
|
if (address[i] != 0xFF) {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
return true;
|
|
}
|
|
|
|
// 168 bytes total for v1
|
|
#pragma pack(push, 1) // Ensure no padding for consistent file storage
|
|
typedef struct {
|
|
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;
|
|
// Only one "input" sender address
|
|
uint8_t senderAddress[32];
|
|
// The "main" recepient address and amount. This is the only required output, and is used for calculating the transaction hash and signature.
|
|
uint8_t recipientAddress1[32];
|
|
// The "extra" recepient address and amount. This can safely be NULL/0 if not used and has multiple uses:
|
|
// - Sending zero: parital spend, sender keeps coins on the same address
|
|
// - Sending to a different address: normal spend, sender's coins move to a new address, e.g. change address
|
|
// - Private Transactions: Can nullify the whole original stealth address input (sender) and send change to a new stealth address (recipient 2) to obfuscate the transaction graph.
|
|
// Note that coinbase will have this as NULL/0 (for now, but we could have multiple payouts in the future)
|
|
uint8_t recipientAddress2[32];
|
|
// Timestamp is dictated by the block
|
|
uint8_t compressedPublicKey[33];
|
|
uint8_t version;
|
|
uint8_t reserved[6]; // 6 bytes (Explicit padding for 8-byte alignment)
|
|
} transaction_t;
|
|
#pragma pack(pop)
|
|
|
|
typedef struct {
|
|
uint8_t signature[64]; // Signature of the hash
|
|
} transaction_sig_t;
|
|
|
|
typedef struct {
|
|
transaction_t transaction;
|
|
transaction_sig_t signature;
|
|
} signed_transaction_t;
|
|
|
|
void Transaction_Init(signed_transaction_t* tx);
|
|
void Transaction_CalculateHash(const signed_transaction_t* tx, uint8_t* outHash);
|
|
void Transaction_Sign(signed_transaction_t* tx, const uint8_t* privateKey);
|
|
bool Transaction_Verify(const signed_transaction_t* tx);
|
|
|
|
#endif
|