Electrum 1.4 leftover and packed P2TR scan - #450
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Three leftover P3s, tests for each contract, required CI green. Unsubscribe, P2TR fail-closed, and the BIP30 TipOnly miss are the right fixes. Two issues on the Electrum 1.4 leftover, one on the write-path BIP30 walk.
Must fix
get_history emits fee on every height <= 0 row, including confirmed genesis (height == 0, fee: None → "fee": 0 via unwrap_or(0)). Electrum 1.4 only puts fee on unconfirmed rows; height 0 is shared by genesis and mempool, and the presence of fee is how clients tell them apart. Gate on i.fee.is_some() (or i.tx_fk.is_null()). The new assertion in dispatch_live_mempool_surfaces only checks height > 0, so genesis would pass today.
Should fix
Unsubscribe does not drop last_sent_status. Subscribe inserts in handle_client; take_new_status only retains against sh_subs on a later notify. A client that rotates gap-limit keys (subscribe → unsubscribe) on a quiet socket never hits that prune, so the map grows past the 1000-sub cap. Remove the hash next to the subscribe insert (or from the unsubscribe arm if you pass last_sent in).
Notes (non-blocking)
- BIP30 walk is correct (
get_all_by_txid+ skip unconnected / same-height self). It sits on the write structural path beforet_spent, with noibd: perftimer, and is per-txid instead of the oldget_fk_by_txid_batch. Pre-BIP34 only, but the repo rule is a named timer on write in the same commit. confirm_run/lookup.rsstill says TipOnly for the pre-BIP34 create-txid stamp wave. That is parent pinning, not the reject, so it does not undo this PR — worth a one-line comment update so the two paths do not look like they still share TipOnly semantics.- P2TR overflow →
Corrupt("output value too large")matchesOutputRecord::decode_at_secret.listunspentheight-1correctly reusesElectrumMempoolItem.height. Asof listunspent still uses the no-mempoolasof_fn. Esplora/utxostill keys mempool offcreate_tx_fk.is_null().
Required checks are green (fmt, deny, clippy, ast-grep, test, windows, macos, multinode, coverage).
| "height": i.height, | ||
| "tx_hash": txid_hex(&i.txid), | ||
| "fee": i.fee.unwrap_or(0), | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
height <= 0 is not the Electrum 1.4 unconfirmed test. Confirmed genesis is also height 0 with fee: None; unwrap_or(0) then advertises "fee": 0 and clients that key off the presence of fee treat it as mempool.
Use i.fee.is_some() (mempool rows from append_mempool_history always set it). Add a genesis get_history pin: confirmed height-0 row must omit fee.
There was a problem hiding this comment.
Done: history_row_json only emits fee when i.fee is Some. Genesis pin is history_row_json_omits_fee_on_confirmed_genesis.
| let sh = param_scripthash(params, 0)?; | ||
| Ok(json!(sh_subs.remove(&sh))) | ||
| } | ||
| "blockchain.scripthash.get_mempool" => { |
There was a problem hiding this comment.
HashSet::remove frees the cap, but last_sent_status in handle_client is only inserted on subscribe and pruned in take_new_status on a later notify. Unsubscribe-all skips emit_sh_notes (sh_subs empty), so a gap-limit rotate on a quiet connection grows that map without bound.
Drop the hash from last_sent_status here (or next to the subscribe insert around line 647) in the same request.
There was a problem hiding this comment.
Done: drop_unsubscribed_status runs after every dispatch so unsubscribe prunes last_sent_status even when no notify follows.
| .store() | ||
| .tx_height_get(old_fk) | ||
| .map_err(ConsensusError::from)? | ||
| == Some(ctx.height.0) | ||
| { | ||
| continue; | ||
| } | ||
| let rec = query.store().get_tx(old_fk).map_err(ConsensusError::from)?; | ||
| let mut unspent = false; | ||
| for v in 0..rec.output_count { | ||
| let spent = query | ||
| .txs | ||
| .get_all_by_txid(txid) | ||
| .map_err(ConsensusError::from)?; | ||
| for (old_fk, rec) in all { |
There was a problem hiding this comment.
Walking every instance is the right BIP30 contract (test covers TipOnly picking the spent newest).
This runs on the write structural path before t_spent, with no named ibd: perf timer, and is one get_all_by_txid per create instead of the old batch TipOnly. Pre-BIP34 only, but lookup/load/write still wants a timer in the same commit (perf_log.rs). A batch get_all is not required if the timer shows this is cheap.
There was a problem hiding this comment.
I don't think this additional walk is necessary because we know that there are no pre bip34 cases where it would be necessary and now bip34 protects us.
There was a problem hiding this comment.
Dropped the extra walk. TipOnly stays: no pre-BIP34 mainnet case needs every instance, and BIP34 covers later duplicates. Rebased onto current master.
There was a problem hiding this comment.
Dropped this walk rather than adding a timer — see the operator note on this thread.
| .store() | ||
| .tx_height_get(old_fk) | ||
| .map_err(ConsensusError::from)? | ||
| == Some(ctx.height.0) | ||
| { | ||
| continue; | ||
| } | ||
| let rec = query.store().get_tx(old_fk).map_err(ConsensusError::from)?; | ||
| let mut unspent = false; | ||
| for v in 0..rec.output_count { | ||
| let spent = query | ||
| .txs | ||
| .get_all_by_txid(txid) | ||
| .map_err(ConsensusError::from)?; | ||
| for (old_fk, rec) in all { |
There was a problem hiding this comment.
I don't think this additional walk is necessary because we know that there are no pre bip34 cases where it would be necessary and now bip34 protects us.
Electrum 1.4 watches leak at the 1000-sub cap with no unsubscribe. Unconfirmed get_history rows omitted fee (get_mempool already had it), and listunspent always reported mempool UTXOs at height 0. Co-authored-by: Cursor <cursoragent@cursor.com>
scan_packed_p2tr_outs mapped uleb128 values above i64::MAX to 0 and kept walking. OutputRecord::decode_at_secret already returns Corrupt for the same bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Electrum 1.4 uses the presence of fee to tell mempool from confirmed genesis (both height 0). Unsubscribe must drop last_sent_status so a quiet gap-limit rotate cannot grow past the 1000-sub cap. Co-authored-by: Cursor <cursoragent@cursor.com>
17b2a3a to
9166aa4
Compare
rearden-grok
Bot
changed the title
1 participant
Summary
Leftover P3 items from the logical-error inventory that are not on
masterand not in open PRs.blockchain.scripthash.unsubscribereturns whether the connection was watching (frees the per-connection cap) and dropslast_sent_statusso a quiet rotate cannot grow past the cap.get_historyunconfirmed rows includefee; confirmed rows (including genesis height 0) omit it.listunspentmempool height is-1when a parent is still in the mempool (otherwise0).scan_packed_p2tr_outsfails closed on an output value abovei64::MAX(Corrupt("output value too large")), matchingOutputRecord::decode_at_secret.Not in this PR: BIP30 all-instance walk. TipOnly is enough: there is no pre-BIP34 mainnet case that needs it, and BIP34 covers later duplicates.
Already done (not in this PR)
masterlist_runsorphan GCclear(), #16 AddrMan capgettxouti64 as u64Corrupts overflowTest plan
cargo test -p rbitcoin-electrum --lib -- scripthash_unsubscribe_frees_cap_slotcargo test -p rbitcoin-electrum --lib -- listunspent_mempool_child_height_is_minus_onecargo test -p rbitcoin-electrum --lib -- history_row_json_omits_fee_on_confirmed_genesiscargo test -p rbitcoin-electrum --lib -- drop_unsubscribed_status_clears_idle_hashescargo test -p rbitcoin-store --lib scan_packed_p2tr_outsfmt,deny,clippy,ast-grep,test,windows,macos,multinode,coverage)