Skip to content

util, swaps: fix nostr announcement PoW mining - #10859

Open
Amperstrand wants to merge 2 commits into
spesmilo:masterfrom
Amperstrand:nostr-pow-fix
Open

util, swaps: fix nostr announcement PoW mining#10859
Amperstrand wants to merge 2 commits into
spesmilo:masterfrom
Amperstrand:nostr-pow-fix

Conversation

@Amperstrand

Copy link
Copy Markdown

Fixes #10858

Summary

Three defects in the swapserver nostr announcement PoW path, all introduced in 947094c1b (PR #9551):

  1. nostr_pow_worker returned the builtin hash instead of the sha256 digest of the winning preimage.
  2. gen_nostr_ann_pow collected the result via done.pop() on the unordered set returned by asyncio.wait(FIRST_COMPLETED). Workers that observe the shutdown event return (None, None) at their next 1M-hash block boundary, and their results can be delivered to the event loop before the winner's own result (independent channels: manager socket vs executor result pipe), so the freshly-mined winning nonce could be silently discarded and (None, 0) propagated.
  3. set_nostr_proof_of_work stored the returned nonce without checking pow_amount >= SWAPSERVER_POW_TARGET. ConfigVar._set_config_value skips its type-check for None, so the race outcome was persisted — the next publish_offer then raised TypeError: hex(None) outside its try/except, killing run_nostr_server (swapserver stops announcing over nostr until restart, then re-mines from scratch).

Changes (+16/−3 across electrum/util.py and electrum/submarine_swaps.py)

  • nostr_pow_worker: return digest, nonce
  • gen_nostr_ann_pow: wait for all workers (ALL_COMPLETED), collect the result that carries a nonce, raise if none does
  • set_nostr_proof_of_work: raise instead of storing a nonce whose achieved work is below the target

Testing

New tests/test_nostr_pow.py (first commit of this PR; the second commit only changes the two source files):

  • The tests intentionally do not run PoW searches — winning nonces are precomputed constants and the process pool / multiprocessing manager are faked — so the suite adds ~0.7 s and no CPU load to CI
  • 5 of the 7 default tests fail on unpatched master (each maps to one defect or its failure mode); all 7 pass with this patch
  • The race is reproduced with faithful fake workers (test_observer_result_completing_first_is_ignored) — deterministic worst-case interleaving, no flakes across repeated runs
  • The 2 passing guards (valid-nonce store, sufficient-pinned-nonce reuse) pin the working behavior; the patch changes nothing about those paths
  • Adjacent suites tests/test_util.py + tests/test_submarine_swaps.py: 64/64 green
  • TestNostrPowSearch runs actual searches and is skipped by default to keep hash loops off CI; locally with ELECTRUM_TESTS_NOSTR_POW=1 python -m pytest tests/test_nostr_pow.py -v it is 9/9 green (~2 s at a 16-bit target)
  • Real-process-pool validation (local): 30/30 runs at target-20 bits return valid nonces with this patch; unpatched master 150/150 valid — the race is real but rare (the winner's result normally leads the observers' by a full 1M-hash block), which the deterministic unit test covers regardless

These tests currently fail: three defects in the PoW path, see next
commit. The tests do not run PoW searches; winning nonces are
precomputed and the multiprocessing machinery is faked. The real
searches in TestNostrPowSearch are skipped unless
ELECTRUM_TESTS_NOSTR_POW is set, to keep hash loops off CI.
Three defects in the swapserver nostr announcement PoW path, all
introduced in 947094c (PR spesmilo#9551):

- nostr_pow_worker returned the builtin hash instead of the sha256
  digest of the winning preimage

- gen_nostr_ann_pow collected the result with done.pop() on the set
  returned by asyncio.wait(FIRST_COMPLETED). Workers that observe the
  shutdown event return (None, None) at their next block boundary, and
  their results can be delivered before the winner's own result, so the
  freshly mined winning nonce could be silently discarded. Wait for all
  workers and collect the result that carries a nonce.

- set_nostr_proof_of_work stored the nonce without checking the achieved
  work against SWAPSERVER_POW_TARGET. A None nonce would later crash
  publish_offer with hex(None), killing run_nostr_server.
@Amperstrand

Copy link
Copy Markdown
Author

Full A/B logs for the record: https://gist.github.com/Amperstrand/4b477e0fbc14b22f8601c601f964b210

  • unpatched master a94e460b5 + tests commit: 5 failed, 2 passed, 2 skipped (deterministic across 3 runs)
  • this branch: 7 passed, 2 skipped (across 5 runs); full default suite pytest tests --ignore=tests/regtest --ignore=tests/qml locally: 1024 passed, 8 skipped, 275 subtests, 0 failures (~4 min)
  • the 2 skipped tests (TestNostrPowSearch) run real process-pool searches and are intentionally gated off CI; locally with ELECTRUM_TESTS_NOSTR_POW=1 they pass 9/9 (~2 s at a 16-bit target). Also ran the real pool directly at target-20 bits: 30/30 valid nonces with the patch.

If a different collection strategy is preferred (e.g. return as soon as any nonce-bearing result completes, instead of waiting for all workers — slightly more code, but zero added startup latency), happy to rework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

swapserver nostr announcement PoW miner: worker returns the builtin hash, gen_nostr_ann_pow can discard the winning nonce, result stored unvalidated

1 participant