From d7b1e8a1d54192df2fcf633fbcdffb43389ff8e2 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 05:42:17 +0000 Subject: pool: guard the share-target floor where the old cap branch was dead ShareTargetFromDifficulty ended with if (t < diff1 && difficulty < 1.0) t = ~uint256(0); which can never fire: t = diff1/difficulty, so difficulty < 1.0 implies t > diff1 and the two conditions exclude each other. The branch was reaching for a real hazard, though, so say the hazard directly instead of deleting the guard: diff1 is ~uint256(0) >> 12, a hair under 2^244, so the true share target stops fitting in a uint256 once difficulty reaches 2^-12 (1/4096), and the fixed-point divisor (uint64)(difficulty * 65536.0) truncates to zero -- a division by zero -- for any difficulty under 2^-16. Past the first threshold getuint256() would return the low 256 bits of an oversized quotient, a wrapped target that is arbitrarily HARDER than difficulty 1. Both thresholds sit far below VARDIFF_MIN (0.001), so nothing live comes near this; the guard exists for any caller that bypasses vardiff. At those difficulties every hash is meant to be a share, so return all-ones, the easiest representable target. Boundary verified numerically against the exact fixed-point algorithm: at difficulty exactly 1/4096 the quotient is 2^256 - 4096 (fits), one ulp below it overflows, and below 2^-16 the divisor is zero. Closes #11 --- src/rpc.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..82f895b 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -115,15 +115,28 @@ static uint256 ShareTargetFromDifficulty(double difficulty) { if (difficulty <= 0.0) difficulty = 1.0; static const uint256 diff1 = CBigNum(bnProofOfWorkLimit).getuint256(); + // Representability floor. shareTarget = diff1 / difficulty, and diff1 is + // ~uint256(0) >> 12, i.e. a hair under 2^244 -- so the true target stops + // fitting in a uint256 once difficulty reaches 2^-12 (= 1/4096), and the + // fixed-point divide below breaks even earlier in a second way: the + // (uint64)(difficulty * 65536.0) divisor truncates to zero for any + // difficulty under 2^-16. Both thresholds sit far below VARDIFF_MIN + // (0.001), so live operation never comes near this, but a caller that + // bypasses vardiff (or a future VARDIFF_MIN) must not divide by zero or + // hand back getuint256()'s low-256-bit truncation of an oversized + // quotient -- a wrapped target is not merely wrong, it is arbitrarily + // HARDER than difficulty 1. At these difficulties every hash is meant to + // be a share, so the honest answer is the easiest representable target. + // (This replaces a dead branch that read `if (t < diff1 && difficulty < + // 1.0)`: t = diff1/difficulty, so difficulty < 1.0 implies t > diff1 and + // the two conditions could never both hold.) + if (difficulty < 1.0 / 4096.0) return ~uint256(0); // shareTarget = diff1 / difficulty CBigNum bn; bn.setuint256(diff1); // Multiply by 2^16 to keep precision, divide, shift back bn <<= 16; bn /= (uint64)(difficulty * 65536.0); - uint256 t = bn.getuint256(); - // Cap at all-ones (maximum, easiest possible) - if (t < diff1 && difficulty < 1.0) t = ~uint256(0); - return t; + return bn.getuint256(); } // Expected number of hashes to find one share at a given Stratum difficulty, -- cgit v1.2.3 From a3666bded1553b101b631d87e99961bf3ccd0a3e Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 05:44:41 +0000 Subject: pool: refuse a (job, nonce) pair this connection already submitted mining.submit validated the nonce, rebuilt the block, re-hashed it with RandomX -- and never asked whether it had seen this exact (job_id, nonce) pair before. Nothing in that pipeline distinguishes a replay from a first submission: a duplicated valid share passes every check and lands in roundShareCount[address]++ a second time, and roundShareCount is the divisor the payout split uses, so replaying one valid share N times multiplies that address's cut of the round by N. Keep a per-connection set of credited nonces on the Miner, reject a repeat with a "Duplicate share" stratum error before the rebuild and hash it would waste, and record the nonce at the moment the share is credited. The set is keyed to the job id the miner is currently working and cleared inside SendJob -- the single funnel every new-job path (initial authorize, block found, new block, 60s refresh, no-job watchdog, vardiff retargets) goes through -- so legitimate resubmissions across jobs are never refused. The clear is keyed on the id rather than unconditional because the vardiff retarget and stall watchdog re-send the CURRENT job, and an unconditional clear there would reopen the very replay window this closes. Job ids come from a monotonic atomic counter in RebuildJob, so an id is never recycled within a process. Closes #8 --- src/rpc.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 82f895b..f1a7153 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -2511,6 +2511,16 @@ struct Miner { int64 lastShareTime = 0; // GetTime() of the previous accepted share double hashRate = 0.0; // smoothed H/s estimate + // Replay guard for mining.submit (#8): nonces already credited on this + // connection for the job it is currently working. Keyed to the job id in + // seenNoncesJob and cleared by SendJob whenever that id changes, so a + // resubmission under a NEW job is never turned away, while a replayed + // (job_id, nonce) pair under the same job is refused before it can be + // credited twice into roundShareCount -- the figure the payout split + // divides by. + std::set seenNonces; + std::string seenNoncesJob; + explicit Miner(btf_socket_t f) : fd(f), connectTime(GetTime()), extranonce2(gNextExtranonce2.fetch_add(1)) {} @@ -2693,6 +2703,17 @@ static json MakeNotifyParams(const StratumJob& job, const CBlock& b, static void SendJob(Miner* m, bool clean) { if (!gHaveJob) return; + // Every path that hands a miner a new job funnels through here, so this + // is the one place the replay guard needs to follow the job id: nonces + // credited under the previous job mean nothing under the new one. Keying + // the clear to the id (rather than clearing unconditionally) keeps a + // same-job re-notify -- the vardiff retarget and stall watchdog both + // re-send the CURRENT job -- from reopening the duplicate window it + // exists to close. + if (m->seenNoncesJob != gCurrentJob.jobId) { + m->seenNonces.clear(); + m->seenNoncesJob = gCurrentJob.jobId; + } CBlock mb = BuildMinerBlock(gCurrentJob, m->extranonce2); // Use this miner's own shareTarget (set by vardiff) in the notify params. // The participant miner reads p[3] as its share target, and HandleLine @@ -2951,6 +2972,22 @@ static bool HandleLine(Miner* m, const std::string& rawLine, } unsigned int nNonce; memcpy(&nNonce, nb.data(), 4); + // Reject a replayed (job, nonce) before it costs a rebuild and a + // RandomX hash: nothing about the work below distinguishes a second + // submission of a valid share from the first, so without this check + // it passes every test and is credited again -- and roundShareCount + // is what the payout split divides by, so each replay multiplies the + // address's cut of the round. The set is per-connection and cleared + // by SendJob on every job change, so only a same-job repeat is + // refused. + if (m->seenNonces.count(nNonce)) { + LogPrint("worker", "[worker->pool] duplicate share from %s" + " nonce=%u job=%s -- already credited, not counting again\n", + m->address.c_str(), nNonce, submitJobId.c_str()); + reply(false, "Duplicate share"); + return true; + } + // Rebuild this miner's exact block (unique merkle via extranonce2) CBlock b = BuildMinerBlock(gCurrentJob, m->extranonce2); b.nNonce = nNonce; @@ -2978,6 +3015,7 @@ static bool HandleLine(Miner* m, const std::string& rawLine, m->roundShares++; m->totalShares++; m->vardiffShares++; + m->seenNonces.insert(nNonce); // replay guard: credit each nonce once per job int64 shareTime = GetTime(); m->lastSeen = shareTime; roundShareCount[m->address]++; -- cgit v1.2.3 From 2bca62a6a9984297633af42d9e19d1f328d2f7f2 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 05:46:57 +0000 Subject: pool: decay a miner's hashrate estimate when its shares stop arriving m->hashRate is an EWMA re-sampled only on the valid-share path, with no time-based term: a miner that stops hashing keeps its last estimate for as long as the connection stays open, and RefreshStats sums every such stale estimate into gStats.totalHashRate, which is what the pool status and the nostr announcement publish. The announced pool hashrate ran several times over ground truth because miners that had gone quiet were still contributing their last sample. Decay on time in the event loop's miner sweep. Per miner, compute the expected share interval -- ExpectedHashesForDifficulty(m->difficulty) divided by its current rate estimate -- and give it a grace of four such intervals with no share (at the vardiff target of one share per 15s that is about a minute of silence). Past the grace, decay the estimate with a half-life of one more expected interval, computed from absolute time so the curve does not depend on the loop's tick rate. The interval is clamped to [1s, 1h] so a fast miner at the vardiff floor, whose expected interval is milliseconds, is not decayed for ordinary share-delivery jitter, and a very slow miner's grace cannot stretch past the point where its estimate means anything. Per-share crediting and ExpectedHashesForDifficulty are untouched: this changes only what is reported about a miner that has stopped reporting itself. Closes #7 --- src/rpc.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index f1a7153..23e25ef 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -2484,6 +2484,26 @@ static const double VARDIFF_MAX = 1e12; static const int64 VARDIFF_STALL_SECS = 45; // 3x share target -- reacts faster without false-triggering on normal variance static const double VARDIFF_STALL_CUT = 4.0; // divide difficulty by this much +// #7: m->hashRate is an EWMA sampled only when a share arrives, so a miner +// that stops hashing keeps its last estimate forever and RefreshStats sums +// it into the announced pool total for as long as the connection sits open +// -- the published pool hashrate ran several times over ground truth +// because of exactly this. The decay pass in PoolEventLoop applies this +// policy: a miner gets a grace of HASHRATE_IDLE_MISSES expected share +// intervals (expected interval = expected hashes per share at its current +// difficulty / its last rate estimate) with no share, then its estimate +// decays with a half-life of one more such interval, computed from +// absolute time so the result does not depend on how often the loop ticks. +// The interval is clamped to [HASHRATE_MIN_INTERVAL, HASHRATE_MAX_INTERVAL] +// so a fast miner at the vardiff floor (expected interval of milliseconds) +// is not punished for ordinary share-delivery jitter, and a very slow +// miner's grace does not stretch past the point where its estimate means +// anything. At the vardiff target of one share per 15s, a stopped miner's +// announced rate is gone within minutes. +static const double HASHRATE_IDLE_MISSES = 4.0; // grace, in expected share intervals +static const double HASHRATE_MIN_INTERVAL = 1.0; // seconds; floor for the expected interval +static const double HASHRATE_MAX_INTERVAL = 3600.0; // seconds; ceiling for the expected interval + struct Miner { btf_socket_t fd; bool authorised = false; @@ -4900,6 +4920,26 @@ static void PoolEventLoop() } } + // 3.6. Hashrate decay (#7) -- see the policy comment at + // HASHRATE_IDLE_MISSES. The EWMA is only re-sampled on share arrival, + // so without this pass a miner that stops hashing keeps its last + // estimate forever and the summed pool hashrate RefreshStats (and + // the nostr announcement built from it) publishes is fiction. + for (Miner* m : gMiners) { + if (m->hashRate <= 0.0 || m->lastShareTime <= 0) continue; + double expected = ExpectedHashesForDifficulty(m->difficulty) / + std::max(m->hashRate, 1e-9); + if (expected < HASHRATE_MIN_INTERVAL) expected = HASHRATE_MIN_INTERVAL; + if (expected > HASHRATE_MAX_INTERVAL) expected = HASHRATE_MAX_INTERVAL; + double silent = (double)(GetTime() - m->lastShareTime); + double grace = HASHRATE_IDLE_MISSES * expected; + if (silent <= grace) continue; + // Half-life of one expected interval past the grace period. + // Written off absolute time, not per-tick, so the decay curve is + // the same however often this loop runs. + m->hashRate *= pow(0.5, (silent - grace) / expected); + } + // 4. Select over miner sockets if (!gMiners.empty()) { fd_set fds; FD_ZERO(&fds); -- cgit v1.2.3 From f5def7625e6369d117cc8f02b7c0244b70d6f779 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 05:49:27 +0000 Subject: pool: reap miners that have said nothing for ten minutes A Miner left gMiners in exactly one place: RecvLine reporting a clean disconnect. Half-open TCP sessions send no FIN and raise no error, so they lived forever -- on the live pool authorize outpaced disconnect 2:1, over half the table never submitted a share, and the leak grew toward MAX_POOL_MINERS, past which the accept path refuses real miners. Two changes. First, an idle reap in the event loop's miner sweep: any miner whose last activity is older than POOL_IDLE_TIMEOUT_SECS (600s) is closed and erased through exactly the bookkeeping the clean-disconnect path uses -- log, sock_close, erase via std::find, delete -- collected into a side vector first so the sweep's iteration stays valid. Shares an idle miner already earned stay in roundShareCount, just as they do when a miner cleanly disconnects mid-round. To give the reap something honest to measure, lastSeen is now stamped on every complete line received (authorize and valid shares already stamped it; a miner that only subscribes, or sends unrecognised methods, previously looked dead from birth), with connectTime standing in for a connection that never completed a line. Ten minutes is far beyond the 15s vardiff share target and the 45s stall watchdog, so no miner that is actually hashing comes close. Second, SO_KEEPALIVE on sockets accepted by the loopback stratum listener, so a half-open session eventually errors out of recv and lands in the reap path on its own. This is a backstop only: the default cadence before the first probe is hours on most stacks, so the idle timeout fires long before keepalive does. Miners arriving via a relay ride a loopback socket pair whose real transport is the rendezvous socket owned by the tunnel pump threads, which keepalive cannot reach across -- for those the idle reap is also what finally tears the tunnel down, since closing the app end unblocks the pumps. Closes #6 --- src/rpc.cpp | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 23e25ef..df7ca05 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -2596,6 +2596,18 @@ static void LocalAcceptFn(void*) break; continue; } + // #6: let the OS probe a peer that has gone silent, so a half-open + // session eventually errors out of recv and lands in the event + // loop's existing reap path instead of sitting in gMiners forever. + // This is a backstop, not the mechanism: the default cadence before + // the first probe is hours on most stacks, so the idle timeout in + // PoolEventLoop fires long before keepalive does. Miners arriving + // via a relay ride a loopback socket pair whose remote transport is + // the rendezvous socket owned by the tunnel pump threads; keepalive + // cannot reach across that, which is part of why the idle timeout + // exists. + int keepalive = 1; + setsockopt(c, SOL_SOCKET, SO_KEEPALIVE, (char*)&keepalive, sizeof(keepalive)); LogPrint("pool", "[pool] miner arrived via loopback -- pushing to event loop\n"); PushReady((btf_socket_t)c); } @@ -2639,6 +2651,16 @@ static const int64 POOL_ACTIVE_WINDOW = 300; // a miner is "active" if it static const int64 POOL_UNAUTH_GRACE_SECS = 60; // a connection still not // authorized after this long is // idle, not merely starting up +// #6: a Miner used to leave gMiners only when RecvLine reported a clean +// disconnect. A half-open session (miner host died, NAT state expired, +// cable pulled) sends no FIN and reports no error, so it sat in the table +// forever -- authorize outpaced disconnect 2:1 on the live pool, and the +// table grew toward MAX_POOL_MINERS, past which real miners are refused. +// lastSeen is stamped on every complete line received; a miner that has +// said nothing for this long is gone in every way that matters. Ten +// minutes is far beyond the vardiff target (a share every 15s) and the +// stall watchdog (45s), so no miner that is actually hashing comes close. +static const int64 POOL_IDLE_TIMEOUT_SECS = 600; // Non-blocking recv: pull whatever bytes are available now into buf, then // return one complete line if one is ready. Three outcomes: @@ -4940,6 +4962,35 @@ static void PoolEventLoop() m->hashRate *= pow(0.5, (silent - grace) / expected); } + // 3.7. Idle reap (#6). Mirror the clean-disconnect path exactly -- + // log, close, erase from gMiners via std::find, delete -- just with + // a different reason. Shares the miner already earned stay in + // roundShareCount, exactly as they do when a miner cleanly + // disconnects mid-round. connectTime stands in for lastSeen on a + // connection that never completed a line. Erasing inside the loop + // above would invalidate the iteration, so collect then reap. + { + int64 now = GetTime(); + std::vector idle; + for (Miner* m : gMiners) { + int64 last = (m->lastSeen > 0) ? m->lastSeen : m->connectTime; + if (now - last > POOL_IDLE_TIMEOUT_SECS) + idle.push_back(m); + } + for (Miner* m : idle) { + LogPrint("worker", "[pool] dropping idle miner %s -- nothing" + " received in over %llds (session shares: %llu," + " round shares: %llu)\n", + m->worker.empty() ? "(never authorized)" : m->worker.c_str(), + (long long)POOL_IDLE_TIMEOUT_SECS, + (unsigned long long)m->sessionShares, + (unsigned long long)m->roundShares); + sock_close(m->fd); + gMiners.erase(std::find(gMiners.begin(), gMiners.end(), m)); + delete m; + } + } + // 4. Select over miner sockets if (!gMiners.empty()) { fd_set fds; FD_ZERO(&fds); @@ -4962,6 +5013,11 @@ static void PoolEventLoop() // terminate the whole node -- not just the pool. Every // line arrives from an unauthenticated socket. while (RecvLine(m->fd, m->readBuf, line, disc)) { + // Any complete line is proof of life (#6): authorize + // and valid shares already stamp lastSeen themselves, + // but a miner that only ever subscribes (or sends + // unrecognised methods) must count as alive too. + m->lastSeen = GetTime(); try { HandleLine(m, line, roundShareCount, roundShareTotal, blocksFoundThisSession); -- cgit v1.2.3 From 0b566b4869b9e86c90162befd87f9882b6dd143c Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 07:09:45 +0000 Subject: net: a refused thread spawn drops one peer, not the node std::thread throws std::system_error when pthread_create is refused (RLIMIT_NPROC, a container's pids cgroup, memory pressure), and nothing caught it, so the node died by std::terminate. Threads are spawned per peer connection, so the pressure that triggers this scales with peer count, which is attacker-influenced. Observed killing a healthy participant worker mid-mining. Every spawn site in the node build, and what a failure now does: - btftunnel.cpp: both tunnel pumps are guarded. The inbound pump starts first because it owns cleanup of the pump end and rendezvous socket; if the second spawn is refused, shutdown() wakes the first out of its read and it cleans up itself -- closing those descriptors from the failure path instead would race it onto reused descriptors. Both paths return INVALID_SOCKET, which every caller already treats as "this connection failed". - btfrv.cpp: the relay's accept loop drops the connection it cannot serve and stops accepting for a moment -- longer after consecutive failures, capped at 5s -- instead of dying at an arbitrary one. The per-pair second pump now runs on the handling thread itself, leaving exactly one spawn to guard and nothing running yet to race. - main.cpp: a participant that cannot spawn its hash workers stops the ones that did start, drops the pool connection, and returns to the reconnect loop, the same way an ordinary disconnect does. - randomx_pow.cpp: dataset init joins the workers it got, releases the dataset, and stays in light mode -- the same trade the allocation failure path above it already makes. - rpc.cpp, net.cpp: the remaining unchecked _beginthread calls (which report failure by value, they cannot throw) now say precisely what did not start. The pool's accept and payout threads failing silently left the pool running deaf -- nobody accepts, nobody gets paid, and nothing in the log says why. Left alone deliberately: the per-relay accept spawns (rpc.cpp) already check, log and retry on the next poll; startup-critical spawns in net.cpp keep failing hard; the selftest-only std::thread uses may abort a selftest run, which is acceptable where dying over one peer is not. Verified: full node rebuilds clean and links; pool-stratum selftest passes; a scratch harness under RLIMIT_NPROC=1 confirms _beginthread reports -1 and std::thread throws system_error, the premises of every path above. --- src/btfrv.cpp | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- src/btftunnel.cpp | 52 ++++++++++++++++++++++++++++++++++++++++++++++++---- src/main.cpp | 30 +++++++++++++++++++++++++++--- src/net.cpp | 5 +++-- src/randomx_pow.cpp | 28 ++++++++++++++++++++++------ src/rpc.cpp | 15 +++++++++++---- 6 files changed, 157 insertions(+), 22 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/btfrv.cpp b/src/btfrv.cpp index 133eb89..8ab735c 100644 --- a/src/btfrv.cpp +++ b/src/btfrv.cpp @@ -40,6 +40,8 @@ typedef int SOCKET; #include #include #include +#include +#include namespace btf { @@ -209,8 +211,28 @@ static void HandleIncoming(SOCKET s) bool a = WriteN(svc, &ok, 1); bool b = WriteN(s, &ok, 1); if (!a || !b) { CLOSESOCK(svc); CLOSESOCK(s); return; } - std::thread(Forward, s, svc).detach(); - std::thread(Forward, svc, s).detach(); + // One thread pumps s -> svc; this thread pumps svc -> s. Spawning a + // thread per direction left no clean way to fail the second spawn: + // std::thread throws std::system_error when pthread_create is refused + // (NPROC limit, container pids cgroup, memory pressure), the first + // pump was already detached, and closing its sockets from here would + // race it -- so nothing caught the throw and it reached + // std::terminate, killing the relay over one pair it could not serve. + // Running one direction on this thread leaves exactly one spawn to + // guard, and a failure here has nothing running yet to race. + try + { + std::thread(Forward, s, svc).detach(); + } + catch (const std::system_error& e) + { + fprintf(stderr, "rendezvous relay: cannot spawn forward thread (%s), pair dropped\n", + e.what()); + CLOSESOCK(svc); + CLOSESOCK(s); + return; + } + Forward(svc, s); } else { @@ -236,6 +258,7 @@ bool RvRelayRun(unsigned short port) if (bind(g_listen, (struct sockaddr*)&addr, sizeof(addr)) != 0) { CLOSESOCK(g_listen); return false; } if (listen(g_listen, 16) != 0) { CLOSESOCK(g_listen); return false; } + int nSpawnFails = 0; while (!g_stop) { struct sockaddr_in cli; @@ -246,7 +269,27 @@ bool RvRelayRun(unsigned short port) if (g_stop) break; continue; } - std::thread(HandleIncoming, s).detach(); + try + { + std::thread(HandleIncoming, s).detach(); + nSpawnFails = 0; + } + catch (const std::system_error& e) + { + // A refused pthread_create is backpressure, not a reason to die: + // the condition behind it (NPROC, pids cgroup, memory) does not + // clear in milliseconds, and the connection count feeding it is + // attacker-influenced. Drop this one peer, stop accepting for a + // moment -- longer each time, capped -- and keep serving the + // connections already established. + fprintf(stderr, "rendezvous relay: cannot spawn connection thread (%s), connection dropped\n", + e.what()); + CLOSESOCK(s); + nSpawnFails++; + int nWaitMs = nSpawnFails < 5 ? nSpawnFails * 1000 : 5000; + for (int i = 0; i < nWaitMs && !g_stop; i += 100) + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } } CLOSESOCK(g_listen); g_listen = INVALID_SOCKET; diff --git a/src/btftunnel.cpp b/src/btftunnel.cpp index 63d178b..2b3f818 100644 --- a/src/btftunnel.cpp +++ b/src/btftunnel.cpp @@ -25,8 +25,10 @@ typedef int SOCKET; #endif #include "sockcount.h" #include +#include #include #include +#include namespace btf { @@ -121,18 +123,60 @@ static void PumpIn(SOCKET pumpEnd, RvSocket rv, std::vector key) // Start the two pumps and hand back the app-facing socket. When both pumps // finish, close the pump end and the rendezvous socket. +// +// Every tunnel is two threads, and a tunnel is one peer connection (a .btf +// dial or accept, a pool miner, a participant's pool session) -- so the +// thread count scales with peer count, which is attacker-influenced. When +// pthread_create is refused std::thread throws std::system_error; uncaught it +// reached std::terminate and took the whole process down over one connection +// it could not serve. Both spawns are guarded, and the failure paths return +// INVALID_SOCKET, which every caller already handles by dropping just that +// connection. static btf_socket_t StartTunnel(RvSocket rv, const unsigned char shared[32]) { btf_socket_t appEnd, pumpEnd; if (!MakeLocalPair(appEnd, pumpEnd)) { RvClose(rv); return INVALID_SOCKET; } std::vector key(shared, shared + 32); - std::thread(PumpOut, pumpEnd, rv, key).detach(); - std::thread([pumpEnd, rv, key]() { - PumpIn(pumpEnd, rv, key); + + // The inbound pump starts first because it owns the cleanup: it closes + // pumpEnd and rv when it exits. That ownership is what makes the second + // spawn's failure path below race-free. + try + { + std::thread([pumpEnd, rv, key]() { + PumpIn(pumpEnd, rv, key); + BtfCloseSocket(pumpEnd); + RvClose(rv); + }).detach(); + } + catch (const std::system_error& e) + { + // Nothing is running yet, so closing everything here is safe. + fprintf(stderr, "btf tunnel: cannot spawn inbound pump (%s), tunnel dropped\n", + e.what()); BtfCloseSocket(pumpEnd); + BtfCloseSocket(appEnd); RvClose(rv); - }).detach(); + return INVALID_SOCKET; + } + + try + { + std::thread(PumpOut, pumpEnd, rv, key).detach(); + } + catch (const std::system_error& e) + { + // The inbound pump is parked in RvReadN(rv). shutdown() wakes it, and + // it closes pumpEnd and rv itself -- closing them here instead would + // race exactly that, and a reused descriptor is how an unrelated + // socket gets closed. + fprintf(stderr, "btf tunnel: cannot spawn outbound pump (%s), tunnel dropped\n", + e.what()); + shutdown((SOCKET)rv, SD_BOTH); + BtfCloseSocket(appEnd); + return INVALID_SOCKET; + } return appEnd; } diff --git a/src/main.cpp b/src/main.cpp index 7b3e555..d154523 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include "bip32.h" #include "btfaddr.h" #include "btftunnel.h" @@ -3899,9 +3900,32 @@ connected: std::vector shareHits; std::mutex shareHitsMtx; std::vector hashThreads; - for (int i = 0; i < nWorkers; i++) - hashThreads.emplace_back(ParticipantHashWorker, &jobShared, i, nWorkers, - &shareHits, &shareHitsMtx); + try + { + for (int i = 0; i < nWorkers; i++) + hashThreads.emplace_back(ParticipantHashWorker, &jobShared, i, nWorkers, + &shareHits, &shareHitsMtx); + } + catch (const std::system_error& e) + { + // A refused pthread_create used to reach std::terminate here and + // kill the worker mid-session -- observed twice on an otherwise + // healthy participant. Stop the workers that did start the same way + // the disconnect path below does, drop this pool connection, and let + // the caller's reconnect loop try again once the pressure clears. + LogPrint("worker", "[worker] cannot spawn hash threads (%s)" + " -- dropping pool connection\n", e.what()); + SetParticipantMiningStatus("hash thread spawn failed"); + { + std::lock_guard lk(jobShared.mtx); + jobShared.stop = true; + jobShared.haveJob = false; + } + for (std::thread& t : hashThreads) + t.join(); + closesocket(s); + return false; + } LogPrint("worker", "[worker] spawned %d hash threads on shared RandomX dataset\n", nWorkers); const uint64 sessionStartHashes = gParticipantHashes.load(); diff --git a/src/net.cpp b/src/net.cpp index fed6123..16038c8 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2090,7 +2090,7 @@ bool StartNode(string& strError) if (addrIncoming.ip) addrLocalHost.ip = addrIncoming.ip; - _beginthread([](void*) { + if (_beginthread([](void*) { unsigned int ip = addrLocalHost.ip; if (GetMyExternalIP(ip)) { addrLocalHost.ip = ip; @@ -2098,7 +2098,8 @@ bool StartNode(string& strError) CWalletDB().WriteSetting("addrIncoming", addrIncoming); printf("External IP updated: %s\n", addrLocalHost.ToStringIP().c_str()); } - }, 0, NULL); + }, 0, NULL) == (uintptr_t)-1) + printf("Error: _beginthread(GetMyExternalIP) failed\n"); // Peers that answered last time, dialled straight away. The relays are the // only way to find anyone otherwise, and reaching them plus walking their diff --git a/src/randomx_pow.cpp b/src/randomx_pow.cpp index 1d058d4..9c59364 100644 --- a/src/randomx_pow.cpp +++ b/src/randomx_pow.cpp @@ -8,6 +8,7 @@ #include #include #include +#include // Fixed key (seed) that determines the RandomX cache/dataset. Keeping it // constant avoids per-epoch "reseed" logic; for the CPU+RAM fairness goal @@ -101,13 +102,28 @@ bool RandomXInitDataset(int nThreads) std::vector workers; unsigned long per = total / nThreads; - for (int i = 0; i < nThreads; i++) + try { - unsigned long start = i * per; - unsigned long count = (i == nThreads - 1) ? (total - start) : per; - workers.emplace_back([start, count]() { - randomx_init_dataset(g_dataset, g_cache, start, count); - }); + for (int i = 0; i < nThreads; i++) + { + unsigned long start = i * per; + unsigned long count = (i == nThreads - 1) ? (total - start) : per; + workers.emplace_back([start, count]() { + randomx_init_dataset(g_dataset, g_cache, start, count); + }); + } + } + catch (const std::system_error& e) + { + // Same refusal as the allocation failure above: light mode is slow + // but alive, std::terminate is neither. Join the workers that did + // start before releasing the dataset out from under them. + for (auto& w : workers) w.join(); + randomx_release_dataset(g_dataset); + g_dataset = NULL; + printf("RandomX: cannot spawn dataset init threads (%s), staying in light mode\n", + e.what()); + return false; } for (auto& w : workers) w.join(); diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..13cb6c9 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -5077,10 +5077,17 @@ void ThreadRPCServer(void*) // armed for its one wallet probe before it is allowed to send. ReconcilePayouts(); - _beginthread(AcceptLoopFn, 0, NULL); - _beginthread(LocalAcceptFn, 0, NULL); - _beginthread(ThreadBtfPoolAnnouncer, 0, NULL); - _beginthread(PayoutThreadFn, 0, NULL); + // _beginthread cannot throw (it reports -1), but an unchecked failure + // leaves the pool running deaf: nobody accepts, announces, or pays, and + // nothing in the log says why. + if (_beginthread(AcceptLoopFn, 0, NULL) == (uintptr_t)-1) + LogPrint("pool", "[pool] FATAL: accept loop thread failed to spawn -- miners cannot connect\n"); + if (_beginthread(LocalAcceptFn, 0, NULL) == (uintptr_t)-1) + LogPrint("pool", "[pool] local accept thread failed to spawn\n"); + if (_beginthread(ThreadBtfPoolAnnouncer, 0, NULL) == (uintptr_t)-1) + LogPrint("pool", "[pool] announcer thread failed to spawn -- pool will not be advertised\n"); + if (_beginthread(PayoutThreadFn, 0, NULL) == (uintptr_t)-1) + LogPrint("pool", "[pool] FATAL: payout thread failed to spawn -- no one gets paid\n"); PoolEventLoop(); // blocks until gPoolRunning = false or fShutdown -- cgit v1.2.3 From f094dc2f95df213f8c89d1d6f25dc864062623ae Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 07:15:45 +0000 Subject: pool: refuse to build a job whose coinbase would burn the subsidy GetKeyFromPool() returns an empty vector when the wallet is encrypted and locked, and when key generation itself fails. RebuildJob() fed whatever came back straight into the coinbase, and CScript::operator<< serialises an empty vector as a zero-length push, so the template's coinbase script became ` OP_CHECKSIG` -- satisfiable by nobody. Every block the pool's miners found against such a job burned its whole subsidy: the coins exist and can never move, while QueuePayouts records the full obligation and the miners are then paid out of coin that block never contributed. An operator who locks the wallet overnight while the miners keep finding blocks pays twice for every one of them. BitcoinMiner has exactly this guard ("no wallet key available for coinbase; mining stopped"); this builder was missed. Mirror it: log the reason and return false before any job state is touched. Every caller already treats a false return as "no job this round" -- the submit handler and the new-block path log a warning and leave gHaveJob false, the no-job watchdog goes on retrying every few seconds, and the periodic nTime refresh keeps broadcasting the old job, whose key was drawn while the wallet could still produce one. Once the wallet can hand out a key again the next retry issues a spendable job unprompted. --- src/rpc.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..cc3fffa 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -223,6 +223,29 @@ static bool RebuildJob() // From the pool, so the template's payout key is in wallet.dat before // any miner is handed work against it. vector vchPubKey = GetKeyFromPool(); + if (vchPubKey.empty()) { + // Empty means the wallet could not produce a key: encrypted and + // locked, or key generation itself failed. Serialised, an empty + // vector is a zero-length push, so the coinbase would become + // ` OP_CHECKSIG` -- a script nobody can satisfy. Every + // block mined against such a job burns its whole subsidy, while + // QueuePayouts still records the full obligation and the miners + // are then paid out of coin that block never contributed. + // BitcoinMiner refuses to mine in this state; refuse to hand the + // pool's miners work in it too. printf and not LogPrint for the + // reason given above the deaf-node refusal, and throttled the + // same way -- the no-job watchdog retries every few seconds for + // as long as the condition lasts. + static int64 nLastSaidNoKey = 0; + if (GetTime() - nLastSaidNoKey > 60) { + nLastSaidNoKey = GetTime(); + printf("RebuildJob: no wallet key available for the coinbase " + "(wallet locked, or key generation failed -- the " + "GetKeyFromPool line above says which) -- not issuing " + "jobs\n"); + } + return false; + } static std::atomic sExtra{0}; uint32_t extraNonce = ++sExtra; -- cgit v1.2.3 From 6c37f59ea136cbace8b0cfccefdad555ff3d619f Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 08:54:01 +0000 Subject: payout: name the funding block in new payout comments A height the pool wins, loses to a reorg, and wins again yields two rounds whose height-only comments are identical -- and identical amounts too when the same miner held both rounds. New sends now write the funding block hash into the comment. The legacy fallback's needle stays frozen at the pre-upgrade format: it exists precisely for transactions that carry nothing else, and a needle in any other format would make every one of those payments look unpaid and pay it again. --- src/rpc.cpp | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 93 insertions(+), 4 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..f9471c2 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1186,11 +1186,35 @@ static bool PayoutBlockStillOnChain(const uint256& blockHash) return false; } -static std::string PayoutComment(int blockHeight) +// The comment every pre-upgrade payout transaction carries, and therefore the +// ONLY needle the legacy fallback in FindRecordedPoolPayout may search with. +// This format is frozen: change it and every payment those builds made stops +// matching, every one of those debts looks unpaid, and reconciliation pays +// them all a second time. +static std::string LegacyPayoutComment(int blockHeight) { return strprintf("Pool payout height %d", blockHeight); } +// The comment written on new sends. It names the funding block, because the +// height alone cannot: a height that is won, lost to a reorg, and won again +// yields two rounds whose height-only comments are identical -- and identical +// amounts too, whenever the same miner held both rounds. The structured +// pool_payout_* metadata already keys the payment by hash for the code; this +// puts the same fact where an operator reading the wallet can see it, and +// makes a new transaction unmatchable by the legacy needle even if the +// structured keys are ever lost. +// +// A zero hash keeps the legacy form: those records predate hash keying, and +// there is no hash to name. +static std::string PayoutComment(int blockHeight, const uint256& blockHash) +{ + if (blockHash == 0) + return LegacyPayoutComment(blockHeight); + return strprintf("Pool payout height %d block %s", blockHeight, + blockHash.ToString().c_str()); +} + // Startup reconciliation. // // This used to be the crash-recovery mechanism: it scanned the wallet with @@ -1425,7 +1449,7 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, CScript want; if (fHaveScript) want << OP_DUP << OP_HASH160 << h160 << OP_EQUALVERIFY << OP_CHECKSIG; - const std::string comment = PayoutComment(blockHeight); + const std::string comment = LegacyPayoutComment(blockHeight); // Candidate legacy matches, adjudicated after cs_mapWallet is released. // @@ -2186,7 +2210,7 @@ static void PayoutThreadFn(void*) } CWalletTx wtx; - wtx.mapValue["comment"] = PayoutComment(r.blockHeight); + wtx.mapValue["comment"] = PayoutComment(r.blockHeight, r.blockHash); // Upstream's structured metadata. This is what makes the wallet // probe above possible at all -- the old comment-only form could // not distinguish two rounds paying the same address the same @@ -3825,7 +3849,7 @@ int RunPayoutLedgerSelfTest() const int height = 8080; const int64 amount = 7 * COIN; const std::string id = PayoutIdForBlock(height, 0); - const std::string comment = PayoutComment(height); + const std::string comment = LegacyPayoutComment(height); std::set none; std::string found; @@ -4419,6 +4443,71 @@ int RunPayoutLedgerSelfTest() } #endif + // -- 14. The payout comment names the funding block ---------------------- + // + // Same height, two hashes: the shape a reorg leaves when the pool wins a + // height, loses the block, and wins the height again. The legacy needle is + // frozen at the pre-upgrade format, so pre-upgrade payments keep matching; + // a new-format comment never matches it, so a new send can never be read + // as a pre-upgrade payment for a twin round. + { + PayoutTestResetState(); + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + + const int height = 9090; + const int64 amount = 3 * COIN; + uint256 hashA = 0, hashB = 0; + hashA.SetHex("00000000000000000000000000000000000000000000000000000000000000aa"); + hashB.SetHex("00000000000000000000000000000000000000000000000000000000000000bb"); + + check(LegacyPayoutComment(height) == + strprintf("Pool payout height %d", height), + "the legacy needle is frozen at the format pre-upgrade builds" + " wrote"); + check(PayoutComment(height, 0) == LegacyPayoutComment(height), + "a grandfathered zero-hash record still writes the legacy form"); + check(PayoutComment(height, hashA) == + strprintf("Pool payout height %d block %s", height, + hashA.ToString().c_str()), + "a hash-keyed record's comment names its funding block"); + check(PayoutComment(height, hashA) != PayoutComment(height, hashB), + "twin rounds at one height carry distinguishable comments"); + + // A transaction wearing the NEW comment format and -- hypothetically + // -- no structured metadata must not be matched by the legacy needle: + // the fallback exists for pre-upgrade transactions, and the new format + // proves this is not one. + const std::string idA = PayoutIdForBlock(height, hashA); + const std::string idB = PayoutIdForBlock(height, hashB); + std::set none; + std::string found; + PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 1, ""); + check(FindRecordedPoolPayout(idB, height, addrA, amount, none, found) + == PAYOUT_PROBE_NONE, + "a new-format comment is never legacy evidence for a twin round"); + check(FindRecordedPoolPayout(idA, height, addrA, amount, none, found) + == PAYOUT_PROBE_NONE, + "nor a match channel for its own round -- structured metadata is"); + + // Round A's own payment, sent by this build, is found by its + // structured id. + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + const std::string paidA = + PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, + 2, idA); + check(FindRecordedPoolPayout(idA, height, addrA, amount, none, found) + == PAYOUT_PROBE_STRUCTURED && found == paidA, + "the winning twin's own payment is found by its structured id"); + + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + } + // -- clean up ------------------------------------------------------------ PayoutTestResetState(); CRITICAL_BLOCK(cs_mapWallet) -- cgit v1.2.3 From a2e1f27b32442bcea92eeda3ac09432f6c5ec2c5 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 08:56:46 +0000 Subject: payout: record the full funding block hash and require it to agree pool_payout_id keeps only the first sixteen hex digits of the block hash, which is the end proof-of-work fills with zeros -- so twin rounds at one height can carry the SAME id, and one twin's payment then settles the other's debt through the structured probe: the same-height collision the comment match had, one layer down. New sends record the full hash as pool_payout_block and a structured match must agree with it. A transaction without the key predates it and is matched by id as before; anything stricter would make every payment already sent look unpaid and pay it again. --- src/rpc.cpp | 96 ++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 20 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index f9471c2..dbbf06d 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1373,10 +1373,15 @@ static void MarkPoolRoundForfeited(const std::string& id, // // Two kinds of evidence, in order of quality. // -// Structured (pool_payout_id / pool_payout_address / pool_payout_amount) is -// upstream's, and it is exact: the id is derived from the block hash, so it -// names one round and one round only. Every transaction this build sends -// carries it. +// Structured (pool_payout_id / pool_payout_address / pool_payout_amount, +// plus pool_payout_block since this series) is upstream's, extended. The id +// is derived from the block hash, but only from the FIRST sixteen hex digits +// of it -- and proof-of-work concentrates zeros exactly there, so the id can +// name two same-height rounds identically. New sends therefore record the +// full hash as pool_payout_block, and a structured match must agree with it +// wherever both sides have one; a transaction without the key was sent +// before it existed, and for those the id is the best evidence there is. +// Every transaction this build sends carries all four keys. // // But NO transaction this pool has ever sent carries it. Every payout in the // existing wallet has only a "comment". So on the first boot after this @@ -1437,6 +1442,7 @@ enum PayoutProbeResult { static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, int blockHeight, + const uint256& blockHash, const std::string& address, int64 amount, const std::set& excluded, @@ -1482,6 +1488,19 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, map::const_iterator amtIt = wtx.mapValue.find("pool_payout_amount"); if (amtIt == wtx.mapValue.end() || amtIt->second != strprintf("%lld", amount)) continue; + // The id alone cannot tell twin rounds at one height apart + // (see above); the full hash can, where the transaction + // recorded one. Where it did not, the id decides, as it + // always has -- refusing those transactions would make every + // payment sent before the key existed look unpaid and pay it + // again. + if (blockHash != 0) { + map::const_iterator blkIt = + wtx.mapValue.find("pool_payout_block"); + if (blkIt != wtx.mapValue.end() + && blkIt->second != blockHash.ToString()) + continue; // a twin round's payment, not this round's + } txidRet = wtx.GetHash().GetHex(); return PAYOUT_PROBE_STRUCTURED; } @@ -2125,7 +2144,8 @@ static void PayoutThreadFn(void*) claimKey, excluded); r.fProbeWallet = false; PayoutProbeResult probe = FindRecordedPoolPayout( - id, r.blockHeight, r.address, r.amount, excluded, recovered); + id, r.blockHeight, r.blockHash, r.address, r.amount, + excluded, recovered); if (probe != PAYOUT_PROBE_NONE) { if (probe == PAYOUT_PROBE_LEGACY) { // Loud on purpose: this is the upgrade path, where the @@ -2219,6 +2239,11 @@ static void PayoutThreadFn(void*) wtx.mapValue["pool_payout_id"] = id; wtx.mapValue["pool_payout_address"] = r.address; wtx.mapValue["pool_payout_amount"] = strprintf("%lld", r.amount); + // The full hash, not only the id's sixteen-digit prefix: the + // prefix of a proof-of-work hash is where the zeros live, and + // the probe needs to tell twin rounds at one height apart. + if (r.blockHash != 0) + wtx.mapValue["pool_payout_block"] = r.blockHash.ToString(); SendMoneyOutcome outcome; if (SendMoney(sc, r.amount, wtx, outcome)) { @@ -3285,7 +3310,8 @@ static std::string PayoutTestAddWalletTx(const std::string& comment, int64 amount, int nonce, const std::string& structuredId, - bool fConfirmed = true) + bool fConfirmed = true, + const std::string& structuredBlock = "") { CWalletTx wtx; wtx.vin.resize(1); @@ -3300,6 +3326,8 @@ static std::string PayoutTestAddWalletTx(const std::string& comment, wtx.mapValue["pool_payout_id"] = structuredId; wtx.mapValue["pool_payout_address"] = address; wtx.mapValue["pool_payout_amount"] = strprintf("%lld", amount); + if (!structuredBlock.empty()) + wtx.mapValue["pool_payout_block"] = structuredBlock; } uint256 hash = wtx.GetHash(); if (fConfirmed) { @@ -3853,30 +3881,30 @@ int RunPayoutLedgerSelfTest() std::set none; std::string found; - check(FindRecordedPoolPayout(id, height, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, none, found) == PAYOUT_PROBE_NONE, "an empty wallet is not evidence that anything was paid"); const std::string legacyTxid = PayoutTestAddWalletTx(comment, addrA, amount, 1, ""); - check(FindRecordedPoolPayout(id, height, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, none, found) == PAYOUT_PROBE_LEGACY && found == legacyTxid, "a payment carrying only a pre-upgrade comment is recognised as" " already paid"); - check(FindRecordedPoolPayout(id, height, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the output to pay THIS recipient"); - check(FindRecordedPoolPayout(id, height, addrA, amount + COIN, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount + COIN, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the output to pay exactly this amount"); - check(FindRecordedPoolPayout(id, height + 1, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height + 1, 0, addrA, amount, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the comment for this block height"); std::set spent; spent.insert(legacyTxid); - check(FindRecordedPoolPayout(id, height, addrA, amount, spent, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, spent, found) == PAYOUT_PROBE_NONE, "one legacy transaction cannot settle a second debt"); @@ -3885,7 +3913,7 @@ int RunPayoutLedgerSelfTest() // exactly what the structured metadata exists to prevent. const std::string otherId = PayoutIdForBlock(height, 0) + "-other"; PayoutTestAddWalletTx(comment, addrB, amount, 2, otherId); - check(FindRecordedPoolPayout(id, height, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) == PAYOUT_PROBE_NONE, "another round's structured payment is not legacy evidence for" " this one"); @@ -3893,7 +3921,7 @@ int RunPayoutLedgerSelfTest() // The structured path still wins where it applies. const std::string structuredTxid = PayoutTestAddWalletTx(comment, addrB, amount, 3, id); - check(FindRecordedPoolPayout(id, height, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) == PAYOUT_PROBE_STRUCTURED && found == structuredTxid, "structured payout metadata is matched in preference to a comment"); @@ -3907,7 +3935,7 @@ int RunPayoutLedgerSelfTest() PayoutTestClearBlockIndex(); const std::string addrC = PayoutTestAddress(0x33); PayoutTestAddWalletTx(comment, addrC, amount, 4, "", false); - check(FindRecordedPoolPayout(id, height, addrC, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, none, found) == PAYOUT_PROBE_NONE, "a pre-upgrade payout that never confirmed is not read as" " payment"); @@ -3917,7 +3945,7 @@ int RunPayoutLedgerSelfTest() mapWallet.clear(); const std::string confirmedTxid = PayoutTestAddWalletTx(comment, addrC, amount, 4, "", true); - check(FindRecordedPoolPayout(id, height, addrC, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, none, found) == PAYOUT_PROBE_LEGACY && found == confirmedTxid, "the same payout, confirmed, is"); @@ -3928,7 +3956,7 @@ int RunPayoutLedgerSelfTest() PayoutTestAddWalletTx(comment, addrC, amount, 5, "", false); std::set spentFirst; spentFirst.insert(confirmedTxid); - check(FindRecordedPoolPayout(id, height, addrC, amount, spentFirst, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, spentFirst, found) == PAYOUT_PROBE_LEGACY && found == alsoConfirmed, "an unconfirmed transaction does not hide a confirmed one behind" " it"); @@ -4484,10 +4512,10 @@ int RunPayoutLedgerSelfTest() std::set none; std::string found; PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 1, ""); - check(FindRecordedPoolPayout(idB, height, addrA, amount, none, found) + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) == PAYOUT_PROBE_NONE, "a new-format comment is never legacy evidence for a twin round"); - check(FindRecordedPoolPayout(idA, height, addrA, amount, none, found) + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) == PAYOUT_PROBE_NONE, "nor a match channel for its own round -- structured metadata is"); @@ -4499,10 +4527,38 @@ int RunPayoutLedgerSelfTest() const std::string paidA = PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 2, idA); - check(FindRecordedPoolPayout(idA, height, addrA, amount, none, found) + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) == PAYOUT_PROBE_STRUCTURED && found == paidA, "the winning twin's own payment is found by its structured id"); + // The id is derived from the FIRST sixteen hex digits of the hash -- + // the end proof-of-work fills with zeros -- so twin hashes that agree + // there collide in id, and a payment carrying no pool_payout_block + // answers for either twin. That is the accepted trade for payments + // sent before the key existed, pinned here so it stays a decision. + check(idA == idB, + "twin hashes differing only past the sixteenth hex digit collide" + " in id -- the reason the full hash must decide"); + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) + == PAYOUT_PROBE_STRUCTURED, + "a payment without the block key answers for a colliding twin --" + " the accepted residual for pre-key transactions"); + + // With the full hash recorded -- as every send now records it -- the + // twins are told apart and the re-won height is paid. + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + const std::string keyedA = + PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, + 3, idA, true, hashA.ToString()); + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) + == PAYOUT_PROBE_STRUCTURED && found == keyedA, + "a payment carrying the full block hash settles its own round"); + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) + == PAYOUT_PROBE_NONE, + "and is no evidence for the twin, so the re-won height is paid"); + CRITICAL_BLOCK(cs_mapWallet) mapWallet.clear(); PayoutTestClearBlockIndex(); -- cgit v1.2.3 From d8eb3aeec198e9e6753b89a53d25f4f91f8ab3d6 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 08:59:21 +0000 Subject: payout: refuse comment-only evidence for a round that has a proof A pre-upgrade payout comment names only the height, so when a height is won, orphaned, and won again, the old transaction matches the new round's (height, address, amount) exactly and the miner is never paid. Round proofs decide the ambiguity where it is decidable: a proof is written when a round is created, and structured payout metadata (upstream 0b01d6c) predates round proofs (upstream b1eb8b6), so a round with a proof was created by a build whose every payment names its round -- a comment-only match can only be another round's money. Skipping the fallback cannot double-pay: any real payment for such a round is already found by the structured probe. Proofless rounds keep the documented bias. --- src/rpc.cpp | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 139 insertions(+), 22 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index dbbf06d..310c0d4 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1369,6 +1369,24 @@ static void MarkPoolRoundForfeited(const std::string& id, PublishPoolRounds(snap); } +// Does this round have a published round proof? +// +// Existence answers a temporal question the legacy fallback below needs: +// which builds could have paid this round. RecordPoolRound writes the proof +// when the round is created, and structured payout metadata (upstream +// cbdd06d) is an ancestor of round proofs (upstream 83ba822) -- no build +// able to write a proof has ever sent a payment that only a comment can +// identify. A round with a proof therefore cannot have a comment-only +// payment; one that matches it is another round's, colliding on the height. +static bool PoolRoundHasProof(const std::string& id) +{ + std::lock_guard lk(gPoolRoundsMutex); + for (const PoolRoundProof& r : gPoolRounds) + if (r.id == id) + return true; + return false; +} + // Which wallet transaction, if any, already paid this? // // Two kinds of evidence, in order of quality. @@ -1404,11 +1422,22 @@ static void MarkPoolRoundForfeited(const std::string& id, // (round, address) pair -- from the ledger's paidTxids, which is durable, // and from what this process has already claimed. One legacy transaction // can therefore settle at most one debt, so N transactions can never -// satisfy more than N obligations. +// satisfy more than N obligations; +// +// 4. it is not consulted at all for a round that has a round proof +// (fAllowLegacy, decided by the caller from PoolRoundHasProof). Proofs +// are written when a round is created, and only by builds whose every +// payment names its round exactly -- so a comment-only transaction +// cannot be a payment for such a round, only a same-height collision +// from an older one, and skipping the fallback cannot hide a real +// payment: the structured probe above already found any that exist. // -// What is left unresolved by construction is two pre-upgrade rounds at the -// SAME height paying the same address the same amount, since the comment -// carries only the height. That needs a reorg. +// What is left unresolved by construction is a comment-only transaction and +// a PROOFLESS round at the same height paying the same address the same +// amount: two grandfathered zero-hash twins, or a twin pair from the builds +// that keyed the ledger by hash before any metadata reached the wallet. The +// comment carries only the height, so nothing ties the transaction to one +// twin rather than the other. That needs a reorg. // // This used to say the losing twin's ledger entry is voided by the orphan // check before it can be paid. That is wrong: PayoutBlockStillOnChain() @@ -1445,6 +1474,7 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, const uint256& blockHash, const std::string& address, int64 amount, + bool fAllowLegacy, const std::set& excluded, std::string& txidRet) { @@ -1505,6 +1535,8 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, return PAYOUT_PROBE_STRUCTURED; } + if (!fAllowLegacy) + continue; // comment-only evidence is inadmissible: rule 4 if (!fHaveScript) continue; // The comment is written only by the payout path, so its presence @@ -2142,10 +2174,15 @@ static void PayoutThreadFn(void*) std::set excluded; CollectForeignPayoutTxids(r.blockHeight, r.blockHash, r.address, claimKey, excluded); + // A round with a published proof was created by a build whose + // every payment names its round exactly, so only structured + // evidence can settle it -- a comment match would be another + // round's payment colliding on the height. + const bool fAllowLegacy = !PoolRoundHasProof(id); r.fProbeWallet = false; PayoutProbeResult probe = FindRecordedPoolPayout( id, r.blockHeight, r.blockHash, r.address, r.amount, - excluded, recovered); + fAllowLegacy, excluded, recovered); if (probe != PAYOUT_PROBE_NONE) { if (probe == PAYOUT_PROBE_LEGACY) { // Loud on purpose: this is the upgrade path, where the @@ -3880,31 +3917,34 @@ int RunPayoutLedgerSelfTest() const std::string comment = LegacyPayoutComment(height); std::set none; std::string found; + // No round proof exists for these rounds, so legacy evidence is + // admissible -- computed exactly as the payout thread computes it. + const bool fAllowLegacy = !PoolRoundHasProof(id); - check(FindRecordedPoolPayout(id, height, 0, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "an empty wallet is not evidence that anything was paid"); const std::string legacyTxid = PayoutTestAddWalletTx(comment, addrA, amount, 1, ""); - check(FindRecordedPoolPayout(id, height, 0, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_LEGACY && found == legacyTxid, "a payment carrying only a pre-upgrade comment is recognised as" " already paid"); - check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the output to pay THIS recipient"); - check(FindRecordedPoolPayout(id, height, 0, addrA, amount + COIN, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount + COIN, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the output to pay exactly this amount"); - check(FindRecordedPoolPayout(id, height + 1, 0, addrA, amount, none, found) + check(FindRecordedPoolPayout(id, height + 1, 0, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "the legacy match requires the comment for this block height"); std::set spent; spent.insert(legacyTxid); - check(FindRecordedPoolPayout(id, height, 0, addrA, amount, spent, found) + check(FindRecordedPoolPayout(id, height, 0, addrA, amount, fAllowLegacy, spent, found) == PAYOUT_PROBE_NONE, "one legacy transaction cannot settle a second debt"); @@ -3913,7 +3953,7 @@ int RunPayoutLedgerSelfTest() // exactly what the structured metadata exists to prevent. const std::string otherId = PayoutIdForBlock(height, 0) + "-other"; PayoutTestAddWalletTx(comment, addrB, amount, 2, otherId); - check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "another round's structured payment is not legacy evidence for" " this one"); @@ -3921,7 +3961,7 @@ int RunPayoutLedgerSelfTest() // The structured path still wins where it applies. const std::string structuredTxid = PayoutTestAddWalletTx(comment, addrB, amount, 3, id); - check(FindRecordedPoolPayout(id, height, 0, addrB, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrB, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_STRUCTURED && found == structuredTxid, "structured payout metadata is matched in preference to a comment"); @@ -3935,7 +3975,7 @@ int RunPayoutLedgerSelfTest() PayoutTestClearBlockIndex(); const std::string addrC = PayoutTestAddress(0x33); PayoutTestAddWalletTx(comment, addrC, amount, 4, "", false); - check(FindRecordedPoolPayout(id, height, 0, addrC, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "a pre-upgrade payout that never confirmed is not read as" " payment"); @@ -3945,7 +3985,7 @@ int RunPayoutLedgerSelfTest() mapWallet.clear(); const std::string confirmedTxid = PayoutTestAddWalletTx(comment, addrC, amount, 4, "", true); - check(FindRecordedPoolPayout(id, height, 0, addrC, amount, none, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_LEGACY && found == confirmedTxid, "the same payout, confirmed, is"); @@ -3956,7 +3996,7 @@ int RunPayoutLedgerSelfTest() PayoutTestAddWalletTx(comment, addrC, amount, 5, "", false); std::set spentFirst; spentFirst.insert(confirmedTxid); - check(FindRecordedPoolPayout(id, height, 0, addrC, amount, spentFirst, found) + check(FindRecordedPoolPayout(id, height, 0, addrC, amount, fAllowLegacy, spentFirst, found) == PAYOUT_PROBE_LEGACY && found == alsoConfirmed, "an unconfirmed transaction does not hide a confirmed one behind" " it"); @@ -4511,11 +4551,14 @@ int RunPayoutLedgerSelfTest() const std::string idB = PayoutIdForBlock(height, hashB); std::set none; std::string found; + // Neither twin has a round proof here (section 15 covers the gate), + // and their ids collide, so one answer serves both. + const bool fAllowLegacy = !PoolRoundHasProof(idA); PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 1, ""); - check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "a new-format comment is never legacy evidence for a twin round"); - check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "nor a match channel for its own round -- structured metadata is"); @@ -4527,7 +4570,7 @@ int RunPayoutLedgerSelfTest() const std::string paidA = PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 2, idA); - check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_STRUCTURED && found == paidA, "the winning twin's own payment is found by its structured id"); @@ -4539,7 +4582,7 @@ int RunPayoutLedgerSelfTest() check(idA == idB, "twin hashes differing only past the sixteenth hex digit collide" " in id -- the reason the full hash must decide"); - check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_STRUCTURED, "a payment without the block key answers for a colliding twin --" " the accepted residual for pre-key transactions"); @@ -4552,10 +4595,10 @@ int RunPayoutLedgerSelfTest() const std::string keyedA = PayoutTestAddWalletTx(PayoutComment(height, hashA), addrA, amount, 3, idA, true, hashA.ToString()); - check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, none, found) + check(FindRecordedPoolPayout(idA, height, hashA, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_STRUCTURED && found == keyedA, "a payment carrying the full block hash settles its own round"); - check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, none, found) + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, fAllowLegacy, none, found) == PAYOUT_PROBE_NONE, "and is no evidence for the twin, so the re-won height is paid"); @@ -4564,6 +4607,80 @@ int RunPayoutLedgerSelfTest() PayoutTestClearBlockIndex(); } + // -- 15. A proof-bearing round rejects comment-only evidence ------------- + // + // The surviving half of the same-height collision: a pre-upgrade + // transaction carries only "Pool payout height H", so if H is won again + // after a reorg, it matches the new round's (height, address, amount) + // exactly and would settle a debt it never paid. The new round has a + // round proof -- written at creation, and only by builds whose payments + // all name their round -- so for it, comment-only evidence is + // inadmissible and the miner is paid. A round without a proof keeps + // today's bias: the evidence is admissible and doubt resolves to + // "already paid". + { + PayoutTestResetState(); + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + const std::string roundsPath = dir + "/pool_rounds_gate_selftest.json"; + const std::string savedRoundsFile = strPoolRoundsFile; + strPoolRoundsFile = roundsPath; + remove(roundsPath.c_str()); + + const int height = 7171; + const int64 amount = 4 * COIN; + uint256 hashB = 0; + hashB.SetHex("00000000000000000000000000000000000000000000000000000000000000cc"); + const std::string idB = PayoutIdForBlock(height, hashB); + std::set none; + std::string found; + + // The colliding pre-upgrade payment: confirmed, comment-only, same + // height, same address, same amount -- the strongest wrong evidence + // there is. + PayoutTestAddWalletTx(LegacyPayoutComment(height), addrA, amount, 1, ""); + + check(!PoolRoundHasProof(idB), + "a round with no recorded proof does not claim one"); + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, + !PoolRoundHasProof(idB), none, found) + == PAYOUT_PROBE_LEGACY, + "without a proof the doubt is genuine and still resolves to" + " already-paid"); + + PoolRoundProof round; + round.id = idB; + round.blockHeight = height; + round.matureAtHeight = height + COINBASE_SPENDABLE_DEPTH; + round.payouts[addrA] = amount; + RecordPoolRound(round); + check(PoolRoundHasProof(idB), + "recording the round proof marks the round as this lineage's"); + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, + !PoolRoundHasProof(idB), none, found) + == PAYOUT_PROBE_NONE, + "with a proof, a same-height comment-only transaction is not" + " evidence, and the re-won height is paid"); + + // Structured evidence still is: the round's own payment, recovered. + const std::string paidB = + PayoutTestAddWalletTx(PayoutComment(height, hashB), addrA, amount, + 2, idB, true, hashB.ToString()); + check(FindRecordedPoolPayout(idB, height, hashB, addrA, amount, + !PoolRoundHasProof(idB), none, found) + == PAYOUT_PROBE_STRUCTURED && found == paidB, + "the gate does not blind the probe to the round's own structured" + " payment"); + + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + remove(roundsPath.c_str()); + remove((roundsPath + ".tmp").c_str()); + strPoolRoundsFile = savedRoundsFile; + } + // -- clean up ------------------------------------------------------------ PayoutTestResetState(); CRITICAL_BLOCK(cs_mapWallet) -- cgit v1.2.3 From 7221bb35d50e9bac11ce5e4b3252c34abe03c124 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 09:01:34 +0000 Subject: payout: treat a malformed ledger block hash as corruption, not as a hash SetHex() parses whatever hex prefix it finds and stops, so a corrupt blockHash loaded as a DIFFERENT hash: garbage with a hex-looking prefix became a hash no block ever had, which the orphan check reads as an orphaned block -- and the next flush voided the debt on the strength of file corruption. Garbage with no hex prefix became 0 and skipped the orphan check instead. A field that is not the 64-digit hex string this tree writes now fails the load, and the file is quarantined like any other parse error. --- src/rpc.cpp | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 2 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 310c0d4..be6c80d 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -776,6 +776,29 @@ static void PublishPendingPayouts(const VersionedBlob& v) gPayoutLedgerPublishedSeq, "pending payout ledger"); } +// Parse a persisted block-hash field, refusing to guess. SetHex() reads +// whatever hex prefix it finds and stops at the first character it does not +// recognise, so a corrupt value does not fail to parse -- it parses as a +// DIFFERENT hash. Both shapes of that are wrong in a dangerous direction: a +// value with a hex-looking prefix ("deadbeef...") becomes a hash no block +// ever had, which PayoutBlockStillOnChain reads as an orphaned block and +// FlushMaturePayouts then VOIDS the debt; a value with no hex prefix at all +// becomes 0, which is grandfathered past the orphan check on the strength +// of garbage. A field that is not the 64-digit hex string ToString() wrote +// is corruption, and corruption is a load failure like any other parse +// error -- the file is quarantined, not read around. +static uint256 ParsePayoutBlockHash(const std::string& str) +{ + if (str.size() != 64 || + str.find_first_not_of("0123456789abcdefABCDEF") != std::string::npos) + throw std::runtime_error( + strprintf("blockHash \"%s\" is not a 64-digit hex string", + str.c_str())); + uint256 h; + h.SetHex(str); + return h; +} + static void LoadPendingPayouts() { std::string path = PayoutFilePath(); @@ -797,7 +820,8 @@ static void LoadPendingPayouts() // field. Leave blockHash zero and they are grandfathered past the // orphan check rather than voided on a guess. if (obj.contains("blockHash") && obj["blockHash"].is_string()) - pp.blockHash.SetHex(obj["blockHash"].get()); + pp.blockHash = + ParsePayoutBlockHash(obj["blockHash"].get()); if (obj.contains("blockHeight") && obj["blockHeight"].is_number()) pp.blockHeight = obj["blockHeight"].get(); // Upstream's tolerance for an entry with no usable height, kept @@ -976,7 +1000,8 @@ static void LoadPaying() r.amount = obj["amount"].get(); r.matureAtHeight = obj["matureAtHeight"].get(); if (obj.contains("blockHash") && obj["blockHash"].is_string()) - r.blockHash.SetHex(obj["blockHash"].get()); + r.blockHash = + ParsePayoutBlockHash(obj["blockHash"].get()); if (obj.contains("blockHeight") && obj["blockHeight"].is_number()) r.blockHeight = obj["blockHeight"].get(); if (obj.contains("retries") && obj["retries"].is_number()) @@ -4681,6 +4706,78 @@ int RunPayoutLedgerSelfTest() strPoolRoundsFile = savedRoundsFile; } + // -- 16. A malformed block hash is corruption, not a hash ---------------- + // + // SetHex() parses whatever hex prefix it finds and stops, so a corrupt + // blockHash did not fail to load -- it loaded as a DIFFERENT hash. + // "deadbeef..." became 0xdeadbeef, a hash no block ever had, which the + // orphan check reads as an orphaned block and the next flush VOIDS the + // debt; a value with no hex prefix became 0, which is grandfathered past + // the orphan check instead. Either way a corrupt file silently changed + // what the pool owed. It is a load failure now, handled like every other + // parse error: quarantine, and never rewrite. + { + PayoutTestResetState(); + remove(ledgerPath.c_str()); + remove(payingPath.c_str()); + + const std::string badLedger = strprintf( + "[{\"amounts\":{\"%s\":100000000}," + "\"blockHash\":\"deadbeef oops, not a hash\"," + "\"blockHeight\":10,\"matureAtHeight\":10}]", addrA.c_str()); + check(PayoutTestWriteFile(ledgerPath, badLedger), + "wrote a ledger whose blockHash is not a 64-digit hex string"); + LoadPendingPayouts(); + { + std::lock_guard lk(gPayoutLedgerMutex); + check(gPendingPayouts.empty(), + "no entry is loaded with a guessed hash"); + } + std::string gone; + check(!PayoutTestReadFile(ledgerPath, gone), + "the ledger with the corrupt hash is quarantined"); + + const std::string badPaying = strprintf( + "[{\"address\":\"%s\",\"amount\":100000000," + "\"blockHash\":\"00000000cafe\",\"blockHeight\":10," + "\"matureAtHeight\":10}]", addrA.c_str()); + check(PayoutTestWriteFile(payingPath, badPaying), + "wrote an in-flight list whose blockHash is truncated"); + LoadPaying(); + { + std::lock_guard lk(gPayoutExecMutex); + check(gPaying.empty(), + "no in-flight record is loaded with a guessed hash"); + } + check(!PayoutTestReadFile(payingPath, gone), + "the in-flight list with the corrupt hash is quarantined"); + + // The two spellings every real file uses still load: the 64-digit + // hash of a keyed round, and the 64 zeros this build writes for a + // grandfathered entry. + PayoutTestResetState(); + remove(ledgerPath.c_str()); + const std::string goodLedger = strprintf( + "[{\"amounts\":{\"%s\":100000000}," + "\"blockHash\":\"00000000000000000000000000000000000000000000000000000000000000cd\"," + "\"blockHeight\":10,\"matureAtHeight\":10}," + "{\"amounts\":{\"%s\":200000000}," + "\"blockHash\":\"%s\"," + "\"blockHeight\":20,\"matureAtHeight\":20}]", + addrA.c_str(), addrB.c_str(), uint256(0).ToString().c_str()); + check(PayoutTestWriteFile(ledgerPath, goodLedger), + "wrote a ledger in both real hash spellings"); + LoadPendingPayouts(); + { + std::lock_guard lk(gPayoutLedgerMutex); + check(gPendingPayouts.size() == 2 && + gPendingPayouts[0].blockHash != 0 && + gPendingPayouts[1].blockHash == 0, + "a real hash and a serialised zero hash both still load"); + } + remove(ledgerPath.c_str()); + } + // -- clean up ------------------------------------------------------------ PayoutTestResetState(); CRITICAL_BLOCK(cs_mapWallet) -- cgit v1.2.3 From 74c068829967bbff60da3506613844df4da1ae80 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 14:14:48 +0000 Subject: mine: ask whether a transaction fits before appending it The node miner tested its size budget only on the outer while loop. The inner for walked all of mapTransactions with no test, so a single pass could append the entire mempool -- the overshoot was bounded by mempool size, not by one transaction as the comment claimed. The pool builder did test, but only after the push, which bounds the overshoot at one transaction. Nothing caps how big one transaction may be: neither CheckTransaction nor AcceptTransaction limits it, so the mempool will hold one of any size the wire allows and "one transaction" is worth up to MAX_SIZE. A lone 1.5 MB transaction offering a large fee is therefore enough to make every template unmineable, and the fee is never paid because the block never confirms. Both now ask before appending and skip what does not fit, so the overshoot is zero and an oversized transaction costs the template nothing rather than invalidating it. The budget moves to MAX_BLOCK_SIZE_GEN so the two assemblers cannot drift on it again -- drift is what produced this. The test is ordered before ConnectInputs on purpose: ConnectInputs credits nFees by reference on success, so declining a transaction after calling it would leave the coinbase claiming a fee the block does not collect. Assembly only. CheckBlock, CheckTransaction and AcceptTransaction are untouched: this changes what we produce, not what the network accepts. --- src/main.cpp | 41 ++++++++++++++++++++++++++++++++++++----- src/main.h | 20 ++++++++++++++++++++ src/rpc.cpp | 34 +++++++++++++++++++++++++--------- 3 files changed, 81 insertions(+), 14 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/main.cpp b/src/main.cpp index 7b3e555..64eb290 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4470,7 +4470,6 @@ bool BitcoinMiner(int nThreadId) map mapTestPool; vector vfAlreadyAdded(mapTransactions.size()); bool fFoundSomething = true; - unsigned int nBlockSize = 0; // Assembly must stop below the consensus cap that CheckBlock() // enforces, or we mine a block our own node then rejects. // @@ -4481,9 +4480,19 @@ bool BitcoinMiner(int nThreadId) // meant solving a block and throwing it away -- with this pool at // ~90% of hashrate, that is the network stalling, not just us. // - // Half the cap, keeping the original margin: the test runs before - // a transaction is added, so the loop can overshoot by one tx. - while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE/2) + // The budget test then sat only on this outer while, and the + // comment claimed that bounded the overshoot to a single + // transaction. It did not. The inner for walks all of + // mapTransactions with no test of its own, so one pass could append + // the entire mempool: the overshoot was bounded by mempool size, + // which is to say not bounded at all. Ask per transaction, before + // appending, and the overshoot is zero rather than small. + // + // Seeded with what the block already weighs -- header, tx count, + // coinbase -- because CheckBlock() weighs the whole serialized + // block and not just the transactions we chose to add to it. + unsigned int nBlockSize = ::GetSerializeSize(*pblock, SER_DISK); + while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE_GEN) { fFoundSomething = false; unsigned int n = 0; @@ -4495,6 +4504,28 @@ bool BitcoinMiner(int nThreadId) if (tx.IsCoinBase() || !tx.IsFinal()) continue; + // Skip, not break: a transaction too big for the room left + // says nothing about the next one, which may still fit. + // + // This is also the only ceiling on a single transaction's + // size anywhere on the path into a block. CheckTransaction + // does not cap it and neither does AcceptTransaction, so + // the mempool will hold one of any size the wire allows; + // without this test a lone 1.5 MB transaction offering a + // fat fee is selected, and every template built from that + // mempool is one CheckBlock() throws away. The fee is never + // paid, because the block never confirms. + // + // Deliberately before ConnectInputs and not after: + // ConnectInputs adds this transaction's fee to nFees by + // reference on success. Passing it and then declining the + // transaction would credit the coinbase for a fee no + // transaction in the block pays, and the block would be + // rejected for a wholly different reason. + unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); + if (!BlockTemplateFits(nBlockSize, nTxSize)) + continue; + // Transaction fee requirements, mainly only needed for flood control // Under 10K (about 80 inputs) is free for first 100 transactions // Base rate is 0.01 per KB @@ -4506,7 +4537,7 @@ bool BitcoinMiner(int nThreadId) swap(mapTestPool, mapTestPoolTmp); pblock->vtx.push_back(tx); - nBlockSize += ::GetSerializeSize(tx, SER_NETWORK); + nBlockSize += nTxSize; vfAlreadyAdded[n] = true; fFoundSomething = true; } diff --git a/src/main.h b/src/main.h index 382dd9c..1f452b5 100644 --- a/src/main.h +++ b/src/main.h @@ -33,6 +33,26 @@ static const unsigned int MAX_BLOCK_SIGOPS = 20000; // limit (32 MB), not a block rule. Bitcoin 0.1.0 predated the later 1 MB block // cap, so this tree inherited no real block-size consensus limit. static const unsigned int MAX_BLOCK_SIZE = 1000000; +// How much of that cap a template we build may fill. Half, which is what both +// assemblers already aimed at -- this only gives the number a name and one +// place to live. They had already drifted on how it was enforced, which is the +// bug this exists to stop repeating. +// +// NOT a consensus rule. CheckBlock() decides what the network accepts; this +// decides what we are willing to produce, and it has to stay under that. +static const unsigned int MAX_BLOCK_SIZE_GEN = MAX_BLOCK_SIZE / 2; + +// Room for one more transaction in a template that currently weighs +// nBlockSize bytes? +// +// Asked BEFORE the transaction is appended, never after. A template that has +// already overshot is one CheckBlock() rejects, and asking afterwards only +// measures the waste. Both assemblers call this, so neither can drift into a +// different idea of what fits. +inline bool BlockTemplateFits(unsigned int nBlockSize, unsigned int nTxSize) +{ + return (nBlockSize + nTxSize <= MAX_BLOCK_SIZE_GEN); +} static const int64 COIN = 100000000; // Total Bitflash emission: identical to Bitcoin (21 million). // MAX_MONEY guards against the value overflow bug (CVE-2010-5139), which in diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..7c5ef6c 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -267,16 +267,24 @@ static bool RebuildJob() map pool; vector vfAlreadyAdded(mapTransactions.size()); bool fFoundSomething = true; - unsigned int sz = 0; - // The cap is MAX_BLOCK_SIZE/2, not MAX_SIZE/2. MAX_SIZE is the - // 32 MB serialisation ceiling, so half of it is 16 MB -- this - // builder would happily assemble a template that its own + // The cap is MAX_BLOCK_SIZE_GEN, not MAX_SIZE/2. MAX_SIZE is + // the 32 MB serialisation ceiling, so half of it is 16 MB -- + // this builder would happily assemble a template that its own // CheckBlock() rejects at the 1 MB consensus cap. This is the // pool's job builder, so a block over the cap is not one // wasted solve, it is every miner on the pool working a - // template the network will reject. The inner break bounds the - // overshoot to a single transaction within a pass. - while (fFoundSomething && sz < MAX_BLOCK_SIZE / 2) + // template the network will reject. + // + // The inner break used to bound the overshoot to one + // transaction per pass. One transaction is still too many: + // nothing caps how big a single transaction may be, so that + // bound was worth up to MAX_SIZE. Asking whether each one fits + // before appending it bounds the overshoot at zero instead. + // + // Seeded with the header, tx count and coinbase, because + // CheckBlock() weighs the whole serialized block. + unsigned int sz = ::GetSerializeSize(block, SER_DISK); + while (fFoundSomething && sz < MAX_BLOCK_SIZE_GEN) { fFoundSomething = false; unsigned int n = 0; @@ -287,16 +295,24 @@ static bool RebuildJob() continue; CTransaction& tx = (*mi).second; if (tx.IsCoinBase() || !tx.IsFinal()) continue; + // Skip rather than stop: one transaction too big for + // the room left does not mean the next is. Ordered + // before ConnectInputs because ConnectInputs credits + // nFees by reference on success, and a transaction we + // then decline would leave the coinbase claiming a fee + // this block does not collect. + unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); + if (!BlockTemplateFits(sz, nTxSize)) + continue; map tmp(pool); if (!tx.ConnectInputs(txdb, tmp, CDiskTxPos(1,1,1), 0, nFees, false, true, tx.GetMinFee(block.vtx.size() < 100))) continue; swap(pool, tmp); block.vtx.push_back(tx); - sz += ::GetSerializeSize(tx, SER_NETWORK); + sz += nTxSize; vfAlreadyAdded[n] = true; fFoundSomething = true; - if (sz > MAX_BLOCK_SIZE / 2) break; } } } -- cgit v1.2.3 From 03a69ccfb36861948e11b0871bb9af8f0b280709 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 14:14:52 +0000 Subject: payout: never collapse a log line that reports money moving LogPrint()'s burst collapse is right for share and peer chatter and wrong for anything that reports an irreversible change to money. A reorg voided 100 ledger entries totalling 4,950 BTF and the log carried one line, naming one block and 49.49 BTF. The other 99 were counted and never printed, and no "(N similar)" summary appeared either -- a summary was only ever flushed when that same call site fired again, and after the reorg it never did. The missing 4,950 BTF was found by counting entries in pending_payouts.json. Add LogPrintAlways(), which skips the collapse entirely, and move the four payout lines that report money to it: an obligation voided, an obligation dropped, a payment sent, a payment that failed. All four are bursty by nature -- one reorg voids every entry funded by the orphaned block, from a single source line -- which is precisely why one line must not stand in for all of them. Their volume is bounded by how much money moved, which is a number worth the lines. Also sweep expired windows for every call site rather than only the one currently logging, so a burst that stops is still summarised, and report the window's real elapsed time instead of a hardcoded 3s. --- src/rpc.cpp | 10 +++--- src/util.cpp | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------ src/util.h | 22 +++++++++++--- 3 files changed, 114 insertions(+), 17 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..2e8f4b8 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1732,7 +1732,9 @@ static void FlushMaturePayouts() } int64 total = 0; for (auto& kv : it->amounts) total += kv.second; - LogPrint("payout", "[payout] block %d (%s) was orphaned -- voiding %zu" + // Always: one line per voided entry, never a collapsed summary. + // A reorg voids these in a burst from this one source line. + LogPrintAlways("payout", "[payout] block %d (%s) was orphaned -- voiding %zu" " payment(s) totalling %s\n", it->blockHeight, it->blockHash.ToString().substr(0, 14).c_str(), it->amounts.size(), FormatMoney(total).c_str()); @@ -2036,7 +2038,7 @@ static void PayoutThreadFn(void*) // funding block can land at any point in that window. Upstream has // no equivalent -- it decides once, when the entry matures. if (!PayoutBlockStillOnChain(r.blockHash)) { - LogPrint("payout", "[payout] block %d (%s) orphaned while queued" + LogPrintAlways("payout", "[payout] block %d (%s) orphaned while queued" " -- dropping %s to %s\n", r.blockHeight, r.blockHash.ToString().substr(0, 14).c_str(), FormatMoney(r.amount).c_str(), r.address.c_str()); @@ -2205,7 +2207,7 @@ static void PayoutThreadFn(void*) // described by at least one of them at every instant. RecordPayoutTxid(r, wtx.GetHash().GetHex()); ForgetPayment(r); - LogPrint("payout", "[payout] paid %s to %s (tx %s)\n", + LogPrintAlways("payout", "[payout] paid %s to %s (tx %s)\n", FormatMoney(r.amount).c_str(), r.address.c_str(), wtx.GetHash().ToString().substr(0, 12).c_str()); } else { @@ -2263,7 +2265,7 @@ static void PayoutThreadFn(void*) // going away must not turn the log into one line a minute // forever, and must not go quiet either. if (r.retries <= 5 || r.retries % 60 == 0) - LogPrint("payout", "[payout] FAILED to pay %s to %s" + LogPrintAlways("payout", "[payout] FAILED to pay %s to %s" " (%s spendable, wallet unlocked, block %d on" " the main chain) -- attempt %d, retrying in" " 60s; this debt stands until it is paid\n", diff --git a/src/util.cpp b/src/util.cpp index 0cd0133..b5b145f 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -47,6 +47,61 @@ struct LogDedupState { static std::map g_logDedup; static CCriticalSection cs_logDedup; +// A pending "(N similar)" summary, lifted out of the table while the lock is +// held and written after it is released. +struct LogDedupFlush { + std::string category; + int suppressed; + int64 elapsed; // how long the window that counted them ran + std::string lastMsg; +}; + +static int64 g_logDedupLastSweep = 0; + +static void EmitDedupSummary(const LogDedupFlush& f) +{ + OutputDebugStringF("[%s] (%d similar in %llds, last: %s)\n", + f.category.c_str(), f.suppressed, + (long long)f.elapsed, f.lastMsg.c_str()); +} + +// A window is otherwise only closed out when its own call site fires again, so +// a burst that simply stops -- 100 ledger entries voided in one pass and then +// silence -- leaves its count sitting in the table and the summary is never +// written at all. That is how 99 of 100 voids went unreported, with no "(N +// similar)" line to say anything was missing (issue #3). Sweep every expired +// window instead, so a finished burst is summarised as soon as anything at all +// is logged. +// +// Expired entries are erased rather than reopened: the window is over, so there +// is no state left worth keeping, and the next call from that site then takes +// the "first time" path and prints directly -- exactly what the rollover branch +// below would have done. It also stops the table growing without bound. +// +// Caller holds cs_logDedup; the summaries are written after it is released. +static void SweepExpiredWindowsLocked(int64 now, std::vector& out) +{ + for (std::map::iterator it = g_logDedup.begin(); + it != g_logDedup.end(); ) + { + if (now - it->second.windowStart < DEDUP_WINDOW_SECS) + { + ++it; + continue; + } + if (it->second.suppressed > 0) + { + LogDedupFlush f; + f.category = it->first.substr(0, it->first.find('|')); + f.suppressed = it->second.suppressed; + f.elapsed = now - it->second.windowStart; + f.lastMsg = it->second.lastMsg; + out.push_back(f); + } + g_logDedup.erase(it++); + } +} + void LogPrintDedup(const char* category, const char* file, int line, const std::string& msgIn) { // Call sites follow the existing printf() convention of ending their @@ -61,11 +116,19 @@ void LogPrintDedup(const char* category, const char* file, int line, const std:: int64 now = GetTime(); bool fEmitNow = false; - int suppressedToFlush = 0; - std::string flushedMsg; + std::vector flushes; CRITICAL_BLOCK(cs_logDedup) { + // At most once a second. The table holds one entry per call site that + // has fired in the last few seconds, so the walk is short, but it must + // not run once per line of a burst. + if (now != g_logDedupLastSweep) + { + g_logDedupLastSweep = now; + SweepExpiredWindowsLocked(now, flushes); + } + std::map::iterator it = g_logDedup.find(key); if (it == g_logDedup.end()) { @@ -76,10 +139,18 @@ void LogPrintDedup(const char* category, const char* file, int line, const std:: } else if (now - it->second.windowStart >= DEDUP_WINDOW_SECS) { - // Window rolled over: flush what was suppressed during it (if - // anything), then start a new window with this call printed. - suppressedToFlush = it->second.suppressed; - flushedMsg = it->second.lastMsg; + // Window rolled over within the same second as the last sweep: + // flush what was suppressed during it (if anything), then start a + // new window with this call printed. + if (it->second.suppressed > 0) + { + LogDedupFlush f; + f.category = category; + f.suppressed = it->second.suppressed; + f.elapsed = now - it->second.windowStart; + f.lastMsg = it->second.lastMsg; + flushes.push_back(f); + } it->second.windowStart = now; it->second.suppressed = 0; it->second.lastMsg = msg; @@ -94,13 +165,23 @@ void LogPrintDedup(const char* category, const char* file, int line, const std:: } } - if (suppressedToFlush > 0) - OutputDebugStringF("[%s] (%d similar in %llds, last: %s)\n", - category, suppressedToFlush, (long long)DEDUP_WINDOW_SECS, flushedMsg.c_str()); + for (size_t i = 0; i < flushes.size(); i++) + EmitDedupSummary(flushes[i]); if (fEmitNow) OutputDebugStringF("[%s] %s\n", category, msg.c_str()); } +// LogPrintAlways() -- see the doc comment in util.h. No window, no counting, +// no table entry: every call writes its own line. +void LogPrintNoCollapse(const char* category, const std::string& msgIn) +{ + std::string msg = msgIn; + while (!msg.empty() && (msg.back() == '\n' || msg.back() == '\r')) + msg.pop_back(); + + OutputDebugStringF("[%s] %s\n", category, msg.c_str()); +} + diff --git a/src/util.h b/src/util.h index d72518b..fe605a1 100644 --- a/src/util.h +++ b/src/util.h @@ -106,15 +106,29 @@ extern bool fDebug; // 2. Burst collapse: repeated LogPrint() calls from the *same source line* // only emit one line per DEDUP_WINDOW_SECS; if more calls land inside // that window they're counted instead of reprinted, and the count is -// flushed as a single "(xN similar in Ws)" line the moment the window -// rolls over. A loop discovering 500 peers by address now produces a -// small, bounded number of lines instead of 500 -- log growth is capped -// by elapsed time, not by how many similar events happen to fire. +// flushed as a single "(xN similar in Ws)" line once the window expires +// -- whether or not that call site ever fires again. A loop discovering +// 500 peers by address now produces a small, bounded number of lines +// instead of 500 -- log growth is capped by elapsed time, not by how +// many similar events happen to fire. +// +// Use LogPrintAlways() instead for anything that reports an irreversible +// change to money: an obligation voided, a payment sent, a payment that +// failed. One line standing in for an unknown number of identical events is +// a tidy report of peer chatter and a false report of a ledger. A reorg +// voiding 100 entries hits one source line 100 times inside one window; +// under LogPrint() that read as a single block losing a single 49.49 BTF +// payment, while 4,950 BTF of obligations were actually destroyed, and the +// discrepancy was only found by counting entries in pending_payouts.json +// (issue #3). Bounded log growth is not worth that ambiguity here. // --------------------------------------------------------------------------- bool LogAcceptsCategory(const char* category); void LogPrintDedup(const char* category, const char* file, int line, const std::string& msg); +void LogPrintNoCollapse(const char* category, const std::string& msg); #define LogPrint(category, ...) \ do { if (LogAcceptsCategory(category)) LogPrintDedup((category), __FILE__, __LINE__, strprintf(__VA_ARGS__)); } while (0) +#define LogPrintAlways(category, ...) \ + do { if (LogAcceptsCategory(category)) LogPrintNoCollapse((category), strprintf(__VA_ARGS__)); } while (0) // Every format string in this project goes through one of the four functions // below -- `printf` itself is remapped to OutputDebugStringF just above, and -- cgit v1.2.3 From 2ed8fae6088c99f1a5dfcfd760c1a24c2b2706db Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 14:16:48 +0000 Subject: mine: count sigops while selecting, not only when validating CheckBlock() caps a block at MAX_BLOCK_SIGOPS and neither assembler counted against it. Bytes do not bound what a block costs to verify: every OP_CHECKSIG is an elliptic-curve verification, so a template comfortably inside the size budget can still be one the network refuses for demanding more work than a block is allowed to. Counted with CTransaction::GetSigOpCount -- the same function CheckBlock() sums, not a second implementation of it -- and seeded from the coinbase, because CheckBlock() sums over all of vtx. The comparison matches too: CheckBlock() fails on `> MAX_BLOCK_SIGOPS`, so a block exactly on the limit is valid and selection accepts it. Assembly and validation differing by one is the shape of the bug this series started with. Unlike the size budget this takes no margin. There is no use for headroom in a sigop count, so assembly holds itself to the consensus number exactly. Assembly only; MAX_BLOCK_SIGOPS and the CheckBlock() test are unchanged. --- src/main.cpp | 19 ++++++++++++++++--- src/main.h | 15 ++++++++++++--- src/rpc.cpp | 13 ++++++++++--- 3 files changed, 38 insertions(+), 9 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/main.cpp b/src/main.cpp index 64eb290..4d8031d 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4492,6 +4492,17 @@ bool BitcoinMiner(int nThreadId) // coinbase -- because CheckBlock() weighs the whole serialized // block and not just the transactions we chose to add to it. unsigned int nBlockSize = ::GetSerializeSize(*pblock, SER_DISK); + // Sigops are a budget too, and CheckBlock() enforces one that no + // assembler here counted against. Bytes do not bound validation + // cost: every OP_CHECKSIG is an elliptic-curve verification, so a + // template well inside the size limit can still be refused for + // demanding more work than a block is allowed to. Counted with + // CTransaction::GetSigOpCount, the function CheckBlock() itself + // sums, so the two cannot disagree about what a transaction costs. + // + // Seeded from the coinbase because CheckBlock() sums over all of + // vtx, coinbase included. + unsigned int nBlockSigOps = pblock->vtx[0].GetSigOpCount(); while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE_GEN) { fFoundSomething = false; @@ -4522,8 +4533,9 @@ bool BitcoinMiner(int nThreadId) // transaction would credit the coinbase for a fee no // transaction in the block pays, and the block would be // rejected for a wholly different reason. - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); - if (!BlockTemplateFits(nBlockSize, nTxSize)) + unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); + unsigned int nTxSigOps = tx.GetSigOpCount(); + if (!BlockTemplateFits(nBlockSize, nBlockSigOps, nTxSize, nTxSigOps)) continue; // Transaction fee requirements, mainly only needed for flood control @@ -4537,7 +4549,8 @@ bool BitcoinMiner(int nThreadId) swap(mapTestPool, mapTestPoolTmp); pblock->vtx.push_back(tx); - nBlockSize += nTxSize; + nBlockSize += nTxSize; + nBlockSigOps += nTxSigOps; vfAlreadyAdded[n] = true; fFoundSomething = true; } diff --git a/src/main.h b/src/main.h index 1f452b5..30e25ef 100644 --- a/src/main.h +++ b/src/main.h @@ -43,15 +43,24 @@ static const unsigned int MAX_BLOCK_SIZE = 1000000; static const unsigned int MAX_BLOCK_SIZE_GEN = MAX_BLOCK_SIZE / 2; // Room for one more transaction in a template that currently weighs -// nBlockSize bytes? +// nBlockSize bytes and spends nBlockSigOps signature operations? // // Asked BEFORE the transaction is appended, never after. A template that has // already overshot is one CheckBlock() rejects, and asking afterwards only // measures the waste. Both assemblers call this, so neither can drift into a // different idea of what fits. -inline bool BlockTemplateFits(unsigned int nBlockSize, unsigned int nTxSize) +// +// Size gets a margin (half the cap); sigops do not. There is no cheap way to +// spend the sigop budget usefully and no reason to reserve headroom in it, so +// assembly holds itself to the same number CheckBlock() enforces -- and to the +// same comparison. CheckBlock() fails on `> MAX_BLOCK_SIGOPS`, so a block +// sitting exactly on the limit is valid and this has to accept it too. +// Assembly and validation disagreeing by one is how the size bug started. +inline bool BlockTemplateFits(unsigned int nBlockSize, unsigned int nBlockSigOps, + unsigned int nTxSize, unsigned int nTxSigOps) { - return (nBlockSize + nTxSize <= MAX_BLOCK_SIZE_GEN); + return (nBlockSize + nTxSize <= MAX_BLOCK_SIZE_GEN && + nBlockSigOps + nTxSigOps <= MAX_BLOCK_SIGOPS); } static const int64 COIN = 100000000; // Total Bitflash emission: identical to Bitcoin (21 million). diff --git a/src/rpc.cpp b/src/rpc.cpp index 7c5ef6c..62eec7a 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -284,6 +284,11 @@ static bool RebuildJob() // Seeded with the header, tx count and coinbase, because // CheckBlock() weighs the whole serialized block. unsigned int sz = ::GetSerializeSize(block, SER_DISK); + // And the sigop budget, which CheckBlock() enforces and this + // builder never counted against. Same counting function + // CheckBlock() sums, seeded from the coinbase because it sums + // over all of vtx. + unsigned int nSigOps = block.vtx[0].GetSigOpCount(); while (fFoundSomething && sz < MAX_BLOCK_SIZE_GEN) { fFoundSomething = false; @@ -301,8 +306,9 @@ static bool RebuildJob() // nFees by reference on success, and a transaction we // then decline would leave the coinbase claiming a fee // this block does not collect. - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); - if (!BlockTemplateFits(sz, nTxSize)) + unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK); + unsigned int nTxSigOps = tx.GetSigOpCount(); + if (!BlockTemplateFits(sz, nSigOps, nTxSize, nTxSigOps)) continue; map tmp(pool); if (!tx.ConnectInputs(txdb, tmp, CDiskTxPos(1,1,1), 0, nFees, @@ -310,7 +316,8 @@ static bool RebuildJob() continue; swap(pool, tmp); block.vtx.push_back(tx); - sz += nTxSize; + sz += nTxSize; + nSigOps += nTxSigOps; vfAlreadyAdded[n] = true; fFoundSomething = true; } -- cgit v1.2.3 From 9a409d8ba97251f29ba80913a4014b8a369c52c0 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 14:46:38 +0000 Subject: pool: advertise a rendezvous only once registered, and log the pin Issue #2 reported that /rvrelay does not govern registration. It does: BtfAllRelays() returns vBtfMeetingRelays alone when fBtfRelayPinned, and every relay-choosing site -- the pool's AcceptLoopFn, the node's ThreadBtfAccept, and the dial path -- draws from it. vBtfMeetingRelays has exactly one writer, the pin itself. A pinned pool therefore spawns one accept thread, at the pin, and no discovered relay is registered at. The registration the incident actually caught was not the pool's. The "registered at" line it quotes exists only in ThreadBtfAccept, and 90.156.222.107:8434 is not a stranger's relay -- it is the third hardcoded seed, one of the three ThreadBtfAccept picks between at random when no pin is in force. That relay is only reachable with the pin absent, so the binary that logged it did not have one; both the pin and the operator-mode skip that keeps a pool out of that thread landed two days before the observation. What was genuinely wrong is that none of this could be read off a log. The pool announced no successful registration at all, and retried failures at one flat line, so "is this pool registered where its descriptor says" had no answer. So: - Advertise the relay after registering at it, not before. The optimistic order published a meeting node we were not yet listed at, and a miner resolving the descriptor inside that window spent its dial on a relay holding no registration for us. ThreadBtfAccept has always ordered it this way; the pool had not. - Log the relay registration succeeds at, name the pin when one is in force, and name each discovered relay the pin excludes. - Escalate when a pinned relay keeps refusing: a pin has no fallback, so an unregistered pin means the pool is unreachable, and that now says so instead of scrolling past. - Refuse to advertise a non-pinned relay in BtfSetActiveRelay. No caller can reach it today, but the descriptor is the one thing a miner obeys, so the invariant is enforced at the sink rather than retrusted to every future caller. Does not touch the registration-consumption race: the pool still gives up its registration on each pairing and re-registers after wrap-and-push. Moving the advertise call inside that loop does not widen the window, it only stops the descriptor naming a relay we never reached. --- src/nostr.cpp | 25 ++++++++++++++++++++++++ src/nostr.h | 3 +++ src/rpc.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 80 insertions(+), 9 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/nostr.cpp b/src/nostr.cpp index e2d839c..0717e12 100644 --- a/src/nostr.cpp +++ b/src/nostr.cpp @@ -151,6 +151,19 @@ string BtfActiveRelay() void BtfSetActiveRelay(const string& relay) { + // With /rvrelay set, the descriptor must never name anything but the pin. + // No caller can reach here with another relay today -- both registration + // paths draw their list from BtfAllRelays(), which returns the pin alone -- + // but the descriptor is the one thing a miner obeys, so the invariant is + // enforced at the sink instead of being retrusted to every future caller. + if (fBtfRelayPinned && + find(vBtfMeetingRelays.begin(), vBtfMeetingRelays.end(), relay) == vBtfMeetingRelays.end()) + { + LogPrint("pool", "refusing to advertise meeting relay %s: /rvrelay pins %s\n", + relay.c_str(), + vBtfMeetingRelays.empty() ? "an empty list" : vBtfMeetingRelays[0].c_str()); + return; + } CRITICAL_BLOCK(cs_activeRelay) strBtfActiveRelay = relay; } @@ -223,6 +236,18 @@ vector BtfAllRelays() return all; } +// Relays learned from Nostr, whether or not a pin is in force. Reporting only: +// it exists so the pool can name what /rvrelay is excluding. Anything choosing +// a relay to register or dial at must keep using BtfAllRelays(). +vector BtfDiscoveredRelays() +{ + vector out; + CRITICAL_BLOCK(cs_discoveredRelays) + foreach(const string& r, g_discoveredRelays) + out.push_back(r); + return out; +} + // // Utilities diff --git a/src/nostr.h b/src/nostr.h index 6dbbe26..0dd919d 100644 --- a/src/nostr.h +++ b/src/nostr.h @@ -60,6 +60,9 @@ extern bool fBtfRelayPinned; bool LooksLikeRelay(const std::string& r); // Every relay to try: curated seeds + relays discovered from Nostr announcements. std::vector BtfAllRelays(); +// Relays discovered from Nostr, ignoring the pin. For reporting what a pin +// excludes -- never for choosing a relay to register or dial at. +std::vector BtfDiscoveredRelays(); struct BtfPoolAnnouncement { diff --git a/src/rpc.cpp b/src/rpc.cpp index a4b53ca..a3560ff 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -4947,13 +4947,19 @@ static void AcceptOneFn(void* arg) std::string host = relay.substr(0, colon); int port = atoi(relay.substr(colon + 1).c_str()); - LogPrint("pool", "[pool] accept thread started for relay %s\n", relay.c_str()); + LogPrint("pool", "[pool] accept thread started for relay %s%s\n", relay.c_str(), + fBtfRelayPinned ? " (/rvrelay pin)" : ""); + + // Registration health for this relay. A pin has no fallback, so a pin that + // never registers means the pool is simply unreachable -- and that has to + // be loud. The 2026-07-30 incident went unnoticed because this loop logged + // failures at one flat line and success not at all, so "which relay is this + // pool actually registered at" could not be answered from the log. + int nConsecFail = 0; + int64 nFirstFail = 0; + bool fEverRegistered = false; while (!fShutdown && gPoolRunning) { - // Advertise this relay as the pool's meeting node BEFORE registering, - // so that miners which resolve our descriptor find a relay where the - // pool (not the node's peer-accept) holds the registration. - BtfSetActiveRelay(relay); // Returns as soon as the relay lists us -- registered, not yet paired. btf::RvSocket rv = btf::RvServiceRegister(host.c_str(), (unsigned short)port, pk); @@ -4962,12 +4968,36 @@ static void AcceptOneFn(void* arg) break; } if (rv == btf::RV_INVALID) { - LogPrint("pool", "[pool] relay %s: RvServiceRegister failed" - " -- backing off 30s before retry\n", relay.c_str()); + nConsecFail++; + if (nFirstFail == 0) nFirstFail = GetTime(); + LogPrint("pool", "[pool] relay %s: registration failed (attempt %d," + " failing for %llds) -- backing off 30s before retry\n", + relay.c_str(), nConsecFail, (long long)(GetTime() - nFirstFail)); + if (fBtfRelayPinned && (nConsecFail == 3 || nConsecFail % 10 == 0)) + LogPrint("pool", "[pool] WARNING: pinned relay %s refused registration" + " %d times running (%llds); /rvrelay leaves no fallback, so" + " miners cannot reach this pool%s\n", + relay.c_str(), nConsecFail, (long long)(GetTime() - nFirstFail), + fEverRegistered ? "" : " -- it has never registered once"); for (int i = 0; i < 30 && !fShutdown && gPoolRunning; i++) Sleep(1000); continue; } + // Advertise THIS relay only now. Setting it before the call published a + // meeting node we were not yet listed at, so a miner resolving the + // descriptor inside that window burned its dial on a relay holding no + // registration for us. net.cpp's peer-accept has always ordered it this + // way round; the pool had not. + BtfSetActiveRelay(relay); + if (!fEverRegistered || nConsecFail > 0) + LogPrint("pool", "[pool] registered at relay %s%s%s -- advertising it as" + " the pool's meeting node\n", relay.c_str(), + fBtfRelayPinned ? " (/rvrelay pin)" : "", + nConsecFail > 0 ? ", recovered" : ""); + fEverRegistered = true; + nConsecFail = 0; + nFirstFail = 0; + // Block for a miner to actually arrive. RvServiceRegister used to do // this itself and returned with the relay's 0x01 pairing byte already // consumed; it no longer does. Without waiting here, BtfServiceWrap @@ -5022,9 +5052,21 @@ static void AcceptLoopFn(void*) LogPrint("pool", "[pool] .btf identity ready after %ds\n", waitSecs * 2); std::set spawned; // relays we've already started a thread for + std::set skipped; // discovered relays the pin excluded, logged once each while (!fShutdown && gPoolRunning) { + // BtfAllRelays() already honours the pin: with /rvrelay set it returns + // vBtfMeetingRelays alone, so no thread is ever spawned for a + // discovered relay. Name the ones it drops -- a pool registering at a + // stranger's relay is exactly the thing that should not be silent. vector relays = BtfAllRelays(); + if (fBtfRelayPinned) { + for (const string& r : BtfDiscoveredRelays()) + if (std::find(relays.begin(), relays.end(), r) == relays.end() && + skipped.insert(r).second) + LogPrint("pool", "[pool] not registering at discovered relay %s" + " -- /rvrelay pins the relay list\n", r.c_str()); + } if (relays.empty()) { LogPrint("pool", "[pool] no relays available -- miners cannot connect yet\n"); } else { @@ -5040,8 +5082,9 @@ static void AcceptLoopFn(void*) relay.c_str()); } else { spawned.insert(relay); - LogPrint("pool", "[pool] accept thread spawned for relay %s" - " (%zu total)\n", relay.c_str(), spawned.size()); + LogPrint("pool", "[pool] accept thread spawned for relay %s%s" + " (%zu total)\n", relay.c_str(), + fBtfRelayPinned ? " (/rvrelay pin)" : "", spawned.size()); } } } -- cgit v1.2.3 From 07051b2378734d62fb94a4a62819d8150452b661 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 15:23:02 +0000 Subject: pool: compute a miner's faded hashrate instead of decaying it in place 2bca62a decayed m->hashRate from the event loop's miner sweep, which turned a figure that is only ever displayed into an accumulator, and got the curve wrong at both ends. Each pass recomputed `expected` -- and so `grace` -- from the estimate it had just decayed. Every decay raised `expected`, which raised the grace that gates the next decay, so `grace` chased `silent` upward and the reported rate settled into a 1/t crawl instead of the half-life the commit's own comment documents: 291x too high at ten half-lives past the grace, and still announcing 0.4% of baseline four hours into a silence. #7 is about the pool over-reporting its hashrate, and this under-corrected it for hours. The far end was worse. Once `expected` reached HASHRATE_MAX_INTERVAL it could not rise any further, the feedback that had been gating each pass was gone, and the decay did begin to compound -- once a second, against the total elapsed silence, because lastShareTime does not advance while a miner is quiet and nothing recorded when the decay had last run. A miner four hours silent fell from 0.4% of its baseline to 1e-11 of it over the next hundred seconds, with nothing in the code or the announced number to explain the cliff. Neither behaviour is worth tuning, so drop the sweep. hashRate is read in exactly one place -- RefreshStats, building the per-worker rows and the total that the pool status and the nostr announcement publish -- and vardiff retargets from share timing alone and never reads it. So leave the stored value as the sample it always was and have RefreshStats ask MinerReportedHashRate() what to publish: a pure function of that sample and the absolute silence since the last share. With no stored decay state there is nothing for a tick rate, a missed pass or a repeated pass to perturb, and the answer is bit-identical from 1/60 Hz to 1000 Hz because nothing accumulates, not because the arithmetic was written carefully. It also keeps the fade out of the estimator: the EWMA on the share path still blends against the value it last computed rather than a faded copy of it, so a miner that resumes recovers exactly as it did before any of this, and there is no decay timestamp for a newly accepted or newly authorised connection to be caught holding unset. RefreshStats takes one GetTime() for the whole sweep, so the rows and the total they are summed into describe a single instant rather than a smear across it. The clamp is written !(expected > MIN) so a NaN lands on the floor instead of sailing through; a negative `silent` from a clock stepping backwards reports the sample unchanged rather than scaling it up; and past HASHRATE_DECAY_CUTOFF half-lives the answer is a flat zero rather than pow() walking down into the denormals. This also drops the std::max(m->hashRate, 1e-9) the old pass carried, which the `hashRate <= 0.0` guard immediately above it had already made dead. pool-stratum gains ten checks over the policy: the full estimate inside the grace, a thousand samples at one instant returning one value and leaving the estimate untouched, a few percent lost one second past the grace rather than the whole estimate, exactly half at one expected interval past it, zero after an hour of silence, no inflation from a backwards clock, zero for a miner that has no rate sample yet, and nothing non-finite anywhere across a long silence. --- src/rpc.cpp | 194 +++++++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 160 insertions(+), 34 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index df7ca05..9d98a4d 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -2488,21 +2488,26 @@ static const double VARDIFF_STALL_CUT = 4.0; // divide difficulty by thi // that stops hashing keeps its last estimate forever and RefreshStats sums // it into the announced pool total for as long as the connection sits open // -- the published pool hashrate ran several times over ground truth -// because of exactly this. The decay pass in PoolEventLoop applies this +// because of exactly this. MinerReportedHashRate() below applies the // policy: a miner gets a grace of HASHRATE_IDLE_MISSES expected share -// intervals (expected interval = expected hashes per share at its current -// difficulty / its last rate estimate) with no share, then its estimate -// decays with a half-life of one more such interval, computed from -// absolute time so the result does not depend on how often the loop ticks. -// The interval is clamped to [HASHRATE_MIN_INTERVAL, HASHRATE_MAX_INTERVAL] -// so a fast miner at the vardiff floor (expected interval of milliseconds) -// is not punished for ordinary share-delivery jitter, and a very slow -// miner's grace does not stretch past the point where its estimate means -// anything. At the vardiff target of one share per 15s, a stopped miner's -// announced rate is gone within minutes. +// intervals (expected interval = expected hashes per share at its +// difficulty, divided by the estimate itself) with no share, after which +// the figure we report falls off with a half-life of one more such +// interval. The interval is clamped to [HASHRATE_MIN_INTERVAL, +// HASHRATE_MAX_INTERVAL] so a fast miner at the vardiff floor (expected +// interval of milliseconds) is not punished for ordinary share-delivery +// jitter, and a very slow miner's grace does not stretch past the point +// where its estimate means anything. At the vardiff target of one share +// per 15s that is a minute of grace, half the rate reported 75s in, and +// nothing reported after about seventeen minutes of silence. static const double HASHRATE_IDLE_MISSES = 4.0; // grace, in expected share intervals static const double HASHRATE_MIN_INTERVAL = 1.0; // seconds; floor for the expected interval static const double HASHRATE_MAX_INTERVAL = 3600.0; // seconds; ceiling for the expected interval +// Half-lives past the grace beyond which we report a flat zero. 2^-64 of the +// original estimate is not a number anyone needs, and stopping here keeps +// pow() from walking down into the denormals for a miner that has been quiet +// for hours. +static const double HASHRATE_DECAY_CUTOFF = 64.0; struct Miner { btf_socket_t fd; @@ -2546,6 +2551,65 @@ struct Miner { extranonce2(gNextExtranonce2.fetch_add(1)) {} }; +// What we publish as a miner's H/s: the last EWMA sample, faded out if shares +// have stopped arriving (#7, policy at HASHRATE_IDLE_MISSES). +// +// This reads m->hashRate and never writes it. That is the whole design. The +// first attempt decayed the stored estimate in place from the event loop's +// sweep, which put a running accumulator behind a figure that is only ever +// displayed, and the accumulator misbehaved in two ways: +// +// - `expected` was recomputed from the already-decayed estimate on every +// pass, so each decay raised `expected`, which raised `grace`, which is +// the very thing gating the next decay. `grace` simply chased `silent` +// upward and the reported rate settled into a 1/t crawl instead of the +// half-life curve the comment above promises -- 291x too high at ten +// half-lives past the grace. Issue #7 is about the pool over-reporting; +// that under-corrected it for hours. +// - Once `expected` reached HASHRATE_MAX_INTERVAL it could not rise any +// further, the feedback that had been gating each pass was gone, and the +// decay began compounding once a second against the total elapsed +// silence -- taking a miner from 0.4% of its baseline to 1e-11 of it in +// the next hundred seconds, four hours in, for no reason a reader of +// either the code or the announced number could see. +// +// Computing the value instead of accumulating it removes the failure mode +// rather than tuning it: with no stored decay state there is nothing for a +// tick rate, a missed pass or a duplicated pass to perturb, and the answer +// depends only on how long the miner has actually been quiet. It also keeps +// the fade out of the estimator -- the EWMA on the share path still blends +// against the value it last computed, not against a faded copy of it, so a +// miner that resumes recovers on exactly the curve it would have without any +// of this. +// +// `now` is a parameter rather than a GetTime() call so that one refresh fades +// every miner to the same instant, and so the policy is directly testable. +static double MinerReportedHashRate(const Miner* m, int64 now) +{ + // Nothing sampled yet: a fresh connection, or one that has found a single + // share and so has no interval to have been timed over. Nothing to fade, + // and nothing to divide by below. + if (m->hashRate <= 0.0 || m->lastShareTime <= 0) return m->hashRate; + + double expected = ExpectedHashesForDifficulty(m->difficulty) / m->hashRate; + // Written so that a NaN would land on the floor rather than sail through: + // the clamp is what guarantees `expected` is finite and strictly positive + // below, whatever the division produced. + if (!(expected > HASHRATE_MIN_INTERVAL)) expected = HASHRATE_MIN_INTERVAL; + if (expected > HASHRATE_MAX_INTERVAL) expected = HASHRATE_MAX_INTERVAL; + + double silent = (double)(now - m->lastShareTime); + double grace = HASHRATE_IDLE_MISSES * expected; + // grace is at least HASHRATE_IDLE_MISSES seconds, so a clock that has + // stepped backwards gives a negative `silent`, takes this branch, and + // reports the sample unchanged. Nothing here can scale an estimate up. + if (!(silent > grace)) return m->hashRate; + + double halflives = (silent - grace) / expected; + if (halflives >= HASHRATE_DECAY_CUTOFF) return 0.0; + return m->hashRate * pow(0.5, halflives); +} + static std::vector gMiners; struct FoundBlock { int height; int64 ts; std::string addr; }; @@ -4672,6 +4736,82 @@ int RunPoolStratumSelfTest() strPoolRoundsFile = savedRoundsFile; gPoolRounds = savedRounds; + // -- hashrate decay (#7) -------------------------------------------------- + // The first version of this decayed m->hashRate in place from the event + // loop's sweep, which made the reported figure a function of how the loop + // happened to run rather than of how long the miner had been quiet. These + // pin the properties that fixes: sampling changes nothing, and the answer + // depends only on elapsed silence. + { + // A miner exactly on the vardiff target: 4096 hashes per share at + // difficulty 1.0, one share per 15s, so 4096/15 H/s. Its expected + // interval is 15s and its grace is 60s. + const double base = 4096.0 / 15.0; + const int64 t0 = 1766000000; // any plausible non-zero wall clock + Miner decayed(INVALID_SOCKET); + decayed.difficulty = 1.0; + decayed.hashRate = base; + decayed.lastShareTime = t0; + + check(MinerReportedHashRate(&decayed, t0 + 60) == base, + "hashrate decay: a miner inside its grace period reports its full estimate"); + + // Sampling must not consume the estimate. Twenty passes is one second + // of the event loop's old sweep; a thousand is nearly a minute of it. + double once = MinerReportedHashRate(&decayed, t0 + 61); + bool idempotent = (decayed.hashRate == base); + for (int i = 0; i < 1000 && idempotent; i++) + idempotent = (MinerReportedHashRate(&decayed, t0 + 61) == once) && + (decayed.hashRate == base); + check(idempotent, + "hashrate decay: repeated sampling returns one value and leaves the estimate alone"); + + // One second past a 60s grace is 1/15th of a half-life: ~4.5% off, not + // the estimate gone. A miner at the vardiff target overshoots its grace + // roughly 1.8% of the time (e^-4) purely by share-timing variance, and + // must not be zeroed for it. + check(once > 0.94 * base && once < 0.96 * base, + "hashrate decay: one second past grace costs a few percent, not the estimate"); + double three = MinerReportedHashRate(&decayed, t0 + 63); + check(three > 0.86 * base && three < 0.88 * base, + "hashrate decay: three seconds past grace costs ~13%"); + + // One half-life past the grace is exactly half. + double half = MinerReportedHashRate(&decayed, t0 + 75); + check(half > 0.499 * base && half < 0.501 * base, + "hashrate decay: one expected interval past grace halves the estimate"); + + // A miner that has genuinely stopped ends up reported as nothing -- + // the whole point of #7. The old in-place version could not get here: + // its decay fed back into its own grace and settled into a 1/t crawl. + check(MinerReportedHashRate(&decayed, t0 + 3600) == 0.0, + "hashrate decay: a miner silent for an hour is reported as zero"); + + // A clock that steps backwards must never inflate what we announce. + check(MinerReportedHashRate(&decayed, t0 - 3600) == base, + "hashrate decay: a backwards clock reports the estimate unchanged, never higher"); + + // Fresh connection, and a connection with one share and so no interval + // to have timed: both must report nothing rather than divide by zero. + Miner fresh(INVALID_SOCKET); + check(MinerReportedHashRate(&fresh, t0) == 0.0, + "hashrate decay: a miner that never submitted a share reports zero"); + Miner single(INVALID_SOCKET); + single.lastShareTime = t0; + check(MinerReportedHashRate(&single, t0 + 86400) == 0.0, + "hashrate decay: a miner with no rate sample yet reports zero"); + + // Nothing in the swept range may produce a NaN, an infinity, a negative + // rate, or a rate above the estimate it started from. + bool finite = true; + for (int64 s = -100; s < 200000 && finite; s += 37) { + double v = MinerReportedHashRate(&decayed, t0 + s); + finite = !std::isnan(v) && !std::isinf(v) && v >= 0.0 && v <= base; + } + check(finite, + "hashrate decay: stays finite, non-negative and non-amplifying across a long silence"); + } + gCurrentJob = savedJob; gHaveJob = savedHaveJob; @@ -4699,6 +4839,10 @@ static void RefreshStats(int blocksFoundThisSession, uint64 roundShareTotal) if (m->authorised) gStats.authorizedMiners++; gWorkerStats.clear(); + // One timestamp for the whole sweep (#7): every miner is faded to the + // same instant, so the per-worker rows and the total they are summed + // into describe one moment rather than a smear across the loop. + int64 nowStats = GetTime(); for (Miner* m : gMiners) { if (!m->authorised || m->lastSeen <= 0) continue; PoolWorkerStatView v; @@ -4707,9 +4851,11 @@ static void RefreshStats(int blocksFoundThisSession, uint64 roundShareTotal) v.roundShares = m->roundShares; v.totalShares = m->totalShares; v.lastSeen = m->lastSeen; - v.hashRate = m->hashRate; + // Not m->hashRate: a miner that has stopped submitting shares must + // not keep contributing its last sample to the announced total. + v.hashRate = MinerReportedHashRate(m, nowStats); gWorkerStats.push_back(v); - gStats.totalHashRate += m->hashRate; + gStats.totalHashRate += v.hashRate; } std::sort(gWorkerStats.begin(), gWorkerStats.end(), [](const PoolWorkerStatView& a, const PoolWorkerStatView& b) { @@ -4942,27 +5088,7 @@ static void PoolEventLoop() } } - // 3.6. Hashrate decay (#7) -- see the policy comment at - // HASHRATE_IDLE_MISSES. The EWMA is only re-sampled on share arrival, - // so without this pass a miner that stops hashing keeps its last - // estimate forever and the summed pool hashrate RefreshStats (and - // the nostr announcement built from it) publishes is fiction. - for (Miner* m : gMiners) { - if (m->hashRate <= 0.0 || m->lastShareTime <= 0) continue; - double expected = ExpectedHashesForDifficulty(m->difficulty) / - std::max(m->hashRate, 1e-9); - if (expected < HASHRATE_MIN_INTERVAL) expected = HASHRATE_MIN_INTERVAL; - if (expected > HASHRATE_MAX_INTERVAL) expected = HASHRATE_MAX_INTERVAL; - double silent = (double)(GetTime() - m->lastShareTime); - double grace = HASHRATE_IDLE_MISSES * expected; - if (silent <= grace) continue; - // Half-life of one expected interval past the grace period. - // Written off absolute time, not per-tick, so the decay curve is - // the same however often this loop runs. - m->hashRate *= pow(0.5, (silent - grace) / expected); - } - - // 3.7. Idle reap (#6). Mirror the clean-disconnect path exactly -- + // 3.6. Idle reap (#6). Mirror the clean-disconnect path exactly -- // log, close, erase from gMiners via std::find, delete -- just with // a different reason. Shares the miner already earned stay in // roundShareCount, exactly as they do when a miner cleanly -- cgit v1.2.3 From b441687b5344454bea16384fe1280e470964faaa Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Thu, 3 Sep 2026 18:31:32 +0000 Subject: payout: move the last two burst-capable money lines to LogPrintAlways Integration review found two more payout lines still on plain LogPrint, both fired per record inside the batch loop in PayoutThreadFn -- the same one-window burst shape as the four already converted. The "writing off" line in ForfeitPayoutRecipient and the "SKIPPED bad address" line at the write-off in PayoutThreadFn both report obligations being destroyed, which is the class of line one summary must never stand in for (issue #3). Message text is unchanged. --- src/rpc.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 2e8f4b8..6531fe8 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1933,7 +1933,7 @@ static void RecordPayoutTxid(const PayingRecord& r, const std::string& txid) // disagreeing. static void ForfeitPayoutRecipient(const PayingRecord& r, const std::string& reason) { - LogPrint("payout", "[payout] writing off %s owed to %s for block %d: %s\n", + LogPrintAlways("payout", "[payout] writing off %s owed to %s for block %d: %s\n", FormatMoney(r.amount).c_str(), r.address.c_str(), r.blockHeight, reason.c_str()); VersionedBlob blob; @@ -2082,7 +2082,7 @@ static void PayoutThreadFn(void*) const std::string reason = strprintf("'%s' is not a valid address, so no transaction" " can pay it", r.address.c_str()); - LogPrint("payout", "[payout] SKIPPED bad address '%s' --" + LogPrintAlways("payout", "[payout] SKIPPED bad address '%s' --" " %s owed for block %d cannot be paid\n", r.address.c_str(), FormatMoney(r.amount).c_str(), r.blockHeight); -- cgit v1.2.3