Repository navigation
Conversation
Closed
…onManager API (Cmd returns instance+response, no err); wire PaymentManager into pool.NewPool/Init (gated on DisablePayment)
There was a problem hiding this comment.
Pull request overview
This PR introduces an initial “payment feature” implementation by wiring a new payments.PaymentManager into pool startup, adding Redis helpers to fetch balances/blocks/contribution data, and adding daemon RPC response types used during payment processing.
Changes:
- Add
payments.PaymentManagerand start it periodically from the pool when payments are enabled. - Add storage helpers for miner balances, pending blocks, and round contributions.
- Add/adjust daemon RPC decode helpers (
gettransaction,sendmany,getbalance,validateaddress) and add payment config options.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/redis.go | Adds Redis accessors used by the payment flow (balances, pending blocks, round contrib). |
| storage/block.go | Introduces block/pending-block parsing helpers used by storage/payment logic. |
| pool/pool.go | Wires the new PaymentManager into pool construction and startup. |
| payments/payer.go | Implements payment processing loop, pending block classification, reward calculation, and sending. |
| payments/payer_test.go | Adds a (currently placeholder) test file for the payments package. |
| daemons/validateaddress.go | Adjusts address validation decoding and error handling. |
| daemons/sendmany.go | Adds decoding for sendmany RPC response. |
| daemons/gettransaction.go | Adds decoding for gettransaction RPC response. |
| daemons/getblocktemplate.go | Updates error handling around getblocktemplate RPC call. |
| daemons/getbalance.go | Adds decoding for getbalance RPC response. |
| config/payment.go | Adds PaymentOptions configuration model. |
| config/options.go | Adds payment config field to top-level options. |
| config/daemon.go | Adjusts daemon String() representation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
139
to
+143
| s := api.NewAPIServer(options, db) | ||
| pm := payments.NewPaymentManager(options.PaymentOptions, options.PoolAddress, dm, db) | ||
|
|
||
| return &Pool{ | ||
| Options: options, | ||
| DaemonManager: dm, | ||
| JobManager: jm, | ||
| APIServer: s, | ||
| Options: options, |
Comment on lines
+36
to
+49
| func NewPaymentManager(options *config.PaymentOptions, poolAddr *config.Recipient, dm *daemons.DaemonManager, db *storage.DB) *PaymentManager { | ||
| pm := &PaymentManager{ | ||
| options: options, | ||
| dm: dm, | ||
|
|
||
| PoolAddress: poolAddr, | ||
| Magnitude: 0, | ||
| MinPayment: 0, | ||
| } | ||
|
|
||
| pm.Init() | ||
|
|
||
| return pm | ||
| } |
Comment on lines
+116
to
+130
| split := strings.Split(string(result.Result), ".") | ||
|
|
||
| s := "1" | ||
| for range split[1] { | ||
| s += "0" | ||
| } | ||
|
|
||
| mul, err := strconv.ParseUint(s, 10, 64) | ||
| if err != nil { | ||
| log.Errorf("cannot parse %s. err: %s", s, err) | ||
| } | ||
| pm.Magnitude = float64(mul) | ||
| pm.MinPayment = pm.CoinToSat(pm.options.MinPayment) | ||
|
|
||
| return nil |
Comment on lines
+188
to
+189
| f64Balances, err := pm.db.GetAllMinerBalances() | ||
| workers := make(map[string]*Worker) |
Comment on lines
+217
to
+240
| pbs := make([]*PendingBlock, len(pendingBlocks)) | ||
| for i, result := range results { | ||
| pbs[i].PendingBlock = pendingBlocks[i] | ||
|
|
||
| if result.Error != nil && result.Error.Code == -5 { | ||
| log.Warnf("Daemon reports invalid transaction: %s", pendingBlocks[i].TxHash) | ||
| pbs[i].Category = storage.Kicked | ||
| continue | ||
| } | ||
|
|
||
| gt, err := daemons.BytesToGetTransaction(result.Result) | ||
| if err != nil { | ||
| log.Error(err) | ||
| continue | ||
| } | ||
|
|
||
| if gt.Details == nil || len(gt.Details) == 0 { | ||
| log.Errorf("Daemon reports no details for transaction: %s", pendingBlocks[i].TxHash) | ||
| continue | ||
| } | ||
|
|
||
| if result.Error != nil || result.Result == nil { | ||
| log.Warnf("Odd error with gettransaction %v", result) | ||
| } |
Comment on lines
100
to
102
| instance, result, _ := dm.Cmd("getblocktemplate", | ||
| []interface{}{map[string]interface{}{"capabilities": []string{"coinbasetxn", "workid", "coinbase/append"}, "rules": rules}}, | ||
| ) |
| err := json.Unmarshal(b, &validateAddress) | ||
| if err != nil { | ||
| log.Fatal(fmt.Sprint("validateAddress call failed with error ", err)) | ||
| return nil, fmt.Errorf("unmashal validateAddress response %s failed with error %s", b, err) |
Comment on lines
+7
to
+10
| func TestNewPaymentManager(t *testing.T) { | ||
| //options := config. | ||
| //dm := daemons.NewDaemonManager() | ||
| } |
| var getBalance GetBalance | ||
| err := json.Unmarshal(b, &getBalance) | ||
| if err != nil { | ||
| return 0.0, fmt.Errorf("unmashal getBalance response %s failed with error %s", b, err) |
| var sendMany SendMany | ||
| err := json.Unmarshal(b, &sendMany) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getTransaction call failed with error %s", err) |
- fix storage key mismatches (round shares / pending blocks written and read consistently); add atomic ApplyPayments (balances, payouts, block moves) - rewrite payer: coinbase maturity check, proportional reward split, balance carry-over, orphan handling, sendmany fee-withhold retry - fork knobs in PaymentOptions: magnitude, minConfirmations, addressCheckMethod, sendManyDummy/omitSendManyDummy (support bitcoind-family forks) - wire PaymentManager.Init error into pool startup - payments test suite via miniredis + a fake wallet daemon (full payout, immature, orphan, below-min carry-over, fork config); example config + docs
…→ assert sendmany pays the miner; wire into run-all + docs
…strings) — fixes payment address check on Bitcoin Core v27+
- prop (default): proportional to the block's round shares - pplns: proportional to the last-N-difficulty share window across rounds (new capped, sequence-scored share log + window mark on each block) - solo: the block finder takes the whole reward (finder recorded per block) - validate payMode at startup; config knobs payMode + pplnsWindow; example + docs - tests for solo and pplns modes via miniredis
- pps: pay a fixed ppsRate per share immediately (credit shares past a cursor from the pplns log), found blocks refill the pool wallet (pool bears variance); validated ppsRate>0 at startup - storage: GetSharesSince/GetPPSCursor + atomic cursor advance in ApplyPayments - payment.sh now runs prop/pplns/solo/pps each against real bitcoind regtest and asserts every mode pays the miner - pps unit tests (credit, block-funds-wallet, cursor, idempotent re-run) + docs
…oesn't drift chain time past the pool's nTime window (fixes pplns/solo/pps 'no block mined')
… catch-up setmocktime froze node time and broke mining for every mode. Instead the E2E now runs each payMode with minConfirmations=1 so a single confirmation block seals a round (sendmany still pays from the mature pre-funded balance), avoiding the 100-block chain-time drift that closed the pool's nTime window between modes. A wait_chain_time helper lets wall-clock catch up to the chain's curtime after the pre-fund and before each mode mines.
classifyBlock reports a coinbase's category from gettransaction, which stays "immature" until the consensus 100-conf maturity. processPayments/processPPS short-circuited on that category, so any minConfirmations below 100 could never pay — defeating the configurable-maturity knob that forks rely on. Gate solely on confirmations >= minConfirmations instead: sendmany funds payouts from any mature wallet UTXO, not the specific (young) coinbase, so a lower threshold is a legitimate operator choice. Unit tests already assert via the confirmations count, so they stay green. e2e: the all-mode payment leg now runs each payMode with minConfirmations=1 (one confirm block per mode, no 100-block chain-time drift) and a tiny pps ppsRate so the many low-vardiff shares the e2eminer logs stay under balance.
settle retried -6 (insufficient funds for fee) by withholding +1% at a time, up to ~99 sendmany calls when the shortfall is large. Make the back-off geometric — a gentle 1% first step for the common fee-only shortfall, then doubling — so it converges in a bounded number of retries. e2e: the e2eminer's shares carry a tiny achieved difficulty (~1e-9), so pps needs a large ppsRate (1e7) to produce a detectable payout that still stays well under the wallet balance. Verified locally: prop/pplns/solo/pps all pay.
…safety, atomicity Fixes found by a local code review of the payment feature: - pplns with the documented default pplnsWindow=0 paid every retained share across earlier rounds (GetPPLNSShares treats <=0 as unbounded); fall back to the block's own round, as documented. - The payment.daemon config index was dead: every wallet RPC hit daemons[0]. Add DaemonManager.CmdToDaemon(index, ...) and route all payer RPCs through it so a split mining/wallet daemon setup works. - Worker names are unauthenticated (AuthorizeFn accepts any), so one malformed name failed the whole sendmany batch and blocked all payouts. Validate each recipient (cached) and skip unpayable ones, carrying their balance over. - ApplyPayments used a non-atomic Pipeline; use TxPipeline (MULTI/EXEC) so a run's balance/payout/block/cursor writes apply all-or-nothing. Adds regression tests for the pplns fallback, the invalid-address skip, and the configured-daemon-index routing. Verified locally: prop/pplns/solo/pps all pay.
…tude Second review pass fixes: - Share.Diff (GBT path) recorded the achieved hash difficulty, so a block- solving share carried ~network difficulty and dominated its PROP/PPLNS round (and gave lucky hashes arbitrary PPS credit). Credit the miner's assigned vardiff instead — the standard unit of share work — which also makes the hashrate stats derived from it correct. E2E pps rate recalibrated accordingly. - DaemonManager wallet RPCs no longer fall through a transport/read/decode error: an unreachable daemon returned a nil response whose body was then dereferenced (crashing the pool), and a garbled body could look like a successful sendmany. Return a nil result so the payer treats it as failure and retries. - setMagnitude no longer infers precision from the getbalance string (JSON drops trailing zeros, so "12.34" wrongly implied magnitude 100); default to the bitcoin-family 1e8, overridable via payment.magnitude.
Vardiff already retargets engine connections (applyEngineVarDiff runs on every valid submit and every engine bakes the per-connection target into its work via JobParamsForDifficulty), but unlike the GBT path it had no previous-difficulty tolerance: a share found against the old target and submitted just after a retarget raised the difficulty was falsely rejected (and fed the ban counter). Track PreviousDifficulty on retarget and honour a submitted share that still meets it, mirroring the GBT prevDiff branch. Engines that never emit ErrLowDiffShare (node-authoritative beam) are unaffected. Generalizes the retarget log label from "ethash" to "engine". Adds a boundary-tolerance test.
Engine-mode clients had no storage handle, so engine coins recorded no shares (the long-standing TODO in the submit handler). Hand the engine stratum server the pool's storage DB and persist every valid/invalid engine share via the same PutShare the GBT path uses: hashrate ZSets, valid/invalid counters, per miner.rig split, round contribution and the PPLNS log now populate for engine coins too. Shares are credited at the assigned difficulty (current, or the previous one for a share tolerated at the vardiff retarget boundary), not the achieved hash difficulty — the same accounting rule as the GBT path. Block recording / automatic payout still depends on the coin's wallet model: engine shares carry no bitcoin-style BlockHex/coinbase txid, so paying engine coins through the bitcoin-family payer remains a per-coin integration.
…cessor Ravencoin is bitcoin-family, so its engine blocks can be paid by the existing PROP/PPLNS/SOLO/PPS processor once they carry a coinbase txid: - Extract CheckBlockAccepted (getblock -> accepted + coinbase txid) onto DaemonManager and reuse it from the GBT JobManager. - kawpow.OnSubmit resolves the coinbase txid after SubmitBlock and sets share.TxHash. - The engine submit path records a payable pending block only when a txid was resolved; other engines' blocks (no txid) stay ordinary share contributions rather than being mis-recorded. - NewEnginePool constructs a PaymentManager (and DaemonManager) when payments are enabled, and Init serves it via a shared startPaymentsIfEnabled helper; non-bitcoin engines fail fast in PaymentManager.Init if payment is enabled. Adds tests: a payable engine block becomes pending (round sealed), a block without a txid stays a share only.
Zcash-family coins are bitcoin-family (t-address) wallets, so equihash blocks can be paid by the same processor as kawpow: resolve the coinbase txid via CheckBlockAccepted after SubmitBlock and set share.TxHash. This completes payout wiring for every bitcoin-family engine; non-bitcoin engines (ethash/cryptonote/ kaspa/ergo/alephium/beam) still leave their blocks as share-only, pending a coin-specific payer.
payment-rvn.sh validates the engine-mode payout integration the GBT payment.sh can't: NewEnginePool constructing and serving the PaymentManager, and the payer validating a real ravend wallet (validateaddress) before classifyBlock/attribute/ sendmany run against real wallet RPCs. kawpow does not reliably land a regtest block (target precision — see gbt.sh), so the block is generated on the node and the pending-block record the pool would have written is injected; the payout half (the engine-pool-specific part) runs for real end-to-end. kawpow's own txid resolution is covered by unit tests and the shared CheckBlockAccepted. Wired into run-all as the RVNPAY leg.
Extend payment-rvn.sh to run the kawpow engine pool once per payMode (prop/pplns/solo/pps), seeding the share state each scheme attributes to a single miner so every mode pays the injected block in full (pps via its per-share credit). Reuses one node-generated block across modes; the engine pool constructs + serves the shared PaymentManager and pays through real ravend wallet RPCs for each mode.
Adds an opt-in e2e-flux CI job (Dockerfile.flux + payment-flux.sh) that proves the equihash engine drives a live fluxd (zcash-style getblocktemplate with coinbasetxn) and the engine pool pays through the shared PaymentManager against a real Flux wallet. Kept separate from the main e2e job because fluxd needs ~775MB of zk params — the main suite stays lean. Skips cleanly when fluxd is absent.
The equihash engine's getblocktemplate at Init failed "time-too-old" because 120 rapidly-generated regtest blocks drifted median-time-past ahead of wall-clock. Generate a short chain and wait for wall-clock to catch up. Refit the leg's assertion to what is genuinely new and reliable — the equihash engine drives a live fluxd (zcash GBT with coinbasetxn) and the engine pool constructs + inits the shared payer against the fluxd wallet. A transparent sendmany can't be funded from coinbase on zcash (shielding), so the actual payout is best-effort/reported; the payout code path itself is proven end-to-end by RVNPAY.
Replace the injected pending-block shortcut with real mining so the leg exercises the full engine-owned path the inject hid: the kawpow engine solves a block, submits it to ravend, resolves the coinbase txid, and PutShare records it as a pending block — then the payer pays the finder. Loops the one-shot miner until the node accepts a block, and surfaces the submitblock reject reason on failure.
The real-mining attempt proved Ravencoin regtest does not run KAWPOW: nodes produce/accept only the legacy 80-byte header, so the pool's mainnet-format 120-byte kawpow block is rejected "Block decode failed (-22)" and can never land (confirmed -kawpowactivationtime has no effect). Real kawpow mining → accepted block is impossible on regtest, so restore the inject-based leg (all four payModes, real ravend wallet + real sendmany) with the reason documented. The engine block-recording path is covered by unit tests + the GBT PAY leg's real mining through the shared CheckBlockAccepted.
Ravencoin regtest hard-codes KAWPOW activation to block time 3582830167 (2083), with no CLI knob — but activation is time-based, so setmocktime past it turns KAWPOW on without recompiling. Pre-fund with legacy blocks for mature sendmany funds, then jump both nodes' clocks past 2083 so the pool mines a real, node-accepted KAWPOW block. This exercises the full engine-owned path inject hid: solve → SubmitBlock → CheckBlockAccepted (real txid) → PutShare pending → payer sendmany-pays the finder.
Ravencoin's CBlockHeader::GetHash() for a KAWPOW block returns KAWPOWHash_OnlyMix — the progpow final hash — not sha256d of the header. The engine set share.BlockHash to reverse(sha256d(header)), a hash the node never stores, so getblock/CheckBlockAccepted could not resolve the coinbase txid and no pending block (hence no payout) was ever recorded on a real solve. Use the digest the PoW already produced. (Surfaced by real-mining the RVNPAY e2e once KAWPOW was activated on regtest via setmocktime.)
…WPOW) The RVN leg previously only ever reported "block candidate submitted (node did not extend the chain)" — it never actually landed a block, because regtest doesn't activate KAWPOW (far-future activation time) so the pool's real kawpow block was rejected "Block decode failed". Activate KAWPOW via setmocktime past the activation time and loop the one-shot engine miner until the chain grows, so "RVN ✅ real block accepted" is genuine and the kawpow block serialization is validated against a live node on the non-payment path too.
…cision" Diagnosis showed monacoind rejects every MONA block "high-hash, proof of work failed": the pool's Lyra2REv2 (bitgoin/lyra2rev2) disagrees with monacoind's, so the pool cannot mine a node-accepted Monacoin block — a real algorithm-library bug the old "node rejected at the target boundary" message hid. Relabel honestly (pipeline validated only up to submission; KNOWN BUG) instead of implying a benign precision issue.
The MONA "node rejected at target boundary" was NOT a lyra2rev2 bug: a new test verifies the pool's Lyra2REv2 against real Monacoin block 2,500,000 (PoW < target) and it matches monacoind. The real cause is the same class as RVN — Monacoin regtest uses scrypt below height 60 (nSwitchLyra2REv2_DGW) and lyra2rev2 above, but the e2e only mined 16 blocks, so the pool's lyra2rev2 block was high-hash to a scrypt-expecting node. Add a configurable prefund block count and mine MONA past height 60 (prefund=70), and surface the reject reason on the fallback so a genuine hash/serialization bug can't hide as "precision" again.
sendmany broadcasts on-chain and ApplyPayments records it in redis, but the two can't share a transaction — a crash between them made the next run re-attribute the same blocks/shares and pay again. Guard it with a durable intent: - settle persists the complete PaymentUpdate (payouts + block/round/cursor moves) BEFORE broadcasting, then stamps the returned txid the instant sendmany returns; ApplyPayments clears the intent in the same atomic commit. - reconcile() runs first each cycle: an intent WITH a txid means the send went out but the state write was interrupted, so it finishes applying it (no re-send); an intent with NO txid is ambiguous (crash mid-RPC), so it halts for operator review rather than risk a double payment. - sendmany errors (incl. the -6 withhold retries) rejected before broadcast clear the intent so normal processing can safely retry. Adds tests: intent cleared on success, sent-intent resumes without re-paying, ambiguous intent halts. Verified locally: all four payModes still pay end-to-end.
Every share was written in its own goroutine (go PutShare), so a block's round seal — Rename(roundCurrent -> round<height>) — was unordered relative to the round-contribution increments of adjacent shares: a share meant for the sealed round could be applied after the Rename (landing in the next round), and vice versa, mis-attributing PROP / round-based PPLNS payouts. Route all shares through one ordered writer: PutShare now enqueues to a buffered channel drained by a single goroutine, so shares apply FIFO and a seal captures exactly the shares recorded before it. Callers (jobManager, engine submit) drop their `go` — PutShare returns at once after enqueueing, so submitting clients still never block on redis. Adds a test asserting pre-block shares (and the block share) land in the sealed round while post-block shares start the next one.
The PPLNS share log is rank-capped at 200k, trimmed on every share. In PPS mode GetSharesSince credits shares above the cursor, but a burst of >200k shares between payout runs (or a payer/redis outage) let the rank trim drop the OLDEST uncredited entries before they were paid — the cursor then advanced past them, underpaying that work. Split retention by mode: the rank cap now applies only outside PPS (PPLNS/PROP/ SOLO), and in PPS the log is trimmed by the payout cursor (ApplyPayments drops shares up to PPSCursor once credited), so it retains exactly the uncredited tail. PaymentManager.Init sets the mode. Adds tests: PPLNS rank-caps, PPS retains past the cap and trims on credit.
classifyBlock orphaned a pending block (irreversibly: moved to orphaned + round deleted) on a single gettransaction -5. But a payment wallet that is syncing or rescanning can transiently not know a valid block's coinbase, so a good block could be dropped and its round lost. On -5, verify the block itself via getblock before orphaning: only orphan if the node doesn't have the block (error) or it's on a side chain (confirmations < 0); if it's on the main chain, leave it pending and retry (with a warning to check the wallet is synced and owns the pool address). Also orphan a reorged coinbase that stays in the wallet but goes conflicted (negative confirmations from gettransaction), which previously lingered pending forever. Adds tests: on-chain miss stays pending, reorged coinbase is orphaned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.