Include onchain transactions in events#448
Conversation
|
While the latest BDK 2.2 now shipped event (which would make emitting events easier on our end), we still need lightningdevkit/rust-lightning#3566 to properly classify the transactions in our store. So this remains blocked until the latter ships (likely with LDK 0.3). |
Is there a way we could get this into ldk-node sooner?
The team seems to lean towards favoring option 3, which is not ideal IMHO, but the main argument is that we don't really have the time to go any other route. The issue that I see from my PoV is that option 2 might even seem faster. Could 2 be an option? |
|
Technically unblocked, but we should first land #791, as the API here will depend on the API choices there. |
b20b0b5 to
b2d7c36
Compare
|
Should be good for review now. |
b2d7c36 to
35a60c4
Compare
35a60c4 to
176ac00
Compare
| if payment_status == PaymentStatus::Pending { | ||
| let pending_payment = | ||
| self.create_pending_payment_from_tx(payment, Vec::new()); | ||
| if updated && payment_status == PaymentStatus::Succeeded { |
There was a problem hiding this comment.
If we crash after the payment_store write but before the event queue is persisted, updated will be false on the next sync upon restart and the event will be lost.
Also from Claude:
This updated gate is the only thing preventing re-emission after a restart (the reloaded store merges the re-derived payment as a no-op), and no test covers it: no restart test syncs and asserts next_event() == None. I wrote that test — it passes, so this is coverage-only.
There was a problem hiding this comment.
Yeah, we applied a somewhat related fix in #948. Maybe it makes sense to first land that then, so we can follow/reuse the exact same pattern here.
| if payment_status == PaymentStatus::Pending { | ||
| let pending_payment = | ||
| self.create_pending_payment_from_tx(stored_payment, Vec::new()); | ||
| self.pending_payment_store.insert_or_update(pending_payment).await?; | ||
| } |
There was a problem hiding this comment.
Directly from Claude:
Direct graduation leaves a pre-existing pending entry stale. Sequence: a tx confirms below ANTI_REORG_DELAY, so a pending entry is written recording its confirmation block; a shallow reorg then moves the tx into a different block; by the next batch sync the tx is ≥ ANTI_REORG_DELAY deep. That sync's TxConfirmed graduates the payment and emits an event with the new (canonical) block, but the pending entry still holds the old, now-orphaned block — and ChainTipChanged then merges that stale entry back in (updated == true), emitting a duplicate event carrying the orphaned block hash and regressing the stored confirmation to the orphaned block. Demonstrated by a failing integration test using a 1-block reorg; no restart needed. Remove/refresh the pending entry when graduating directly — with that fixed, emission is exactly-once for reorgs shallower than ANTI_REORG_DELAY, which is worth documenting on the event variants.
| }; | ||
|
|
||
| generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; | ||
| wait_for_tx(&electrsd.client, txid).await; |
There was a problem hiding this comment.
The CI failure is here according to Claude:
This ordering is what failed CI (build (self-hosted, beta)) once the new assertion below started depending on it: send() only queues the broadcast (drained by a background task), and wait_for_tx after mining also passes for a mempool-only tx — so the tx can miss the first of the 6 blocks, sit below ANTI_REORG_DELAY at the sync, and never produce the event (expect_onchain_payment_event can't recover; single 60s next_event_async, tests/common/mod.rs:426-434). Swap these two lines, as onchain_send_receive does.
| assert_eq!(node_a.next_event(), None); | ||
| assert_eq!(node_b.next_event(), None); |
There was a problem hiding this comment.
Claude:
After the reorg sync below, also assert no event, then re-mine to ANTI_REORG_DELAY and assert exactly one event fires. Extended that way, it currently fails due to the stale-pending-entry duplicate (src/wallet/mod.rs:289).
|
Tests mentioned in claude feedback can be found in jkczyz@a7c0269 |
Let callers obtain the object produced by merge semantics without a second lookup. Preserve the existing boolean API so unrelated call sites and their allocation behavior remain unchanged. Co-Authored-By: HAL 9000
Exercise the new helper's insertion path as well as its update and no-op paths so its returned effective object stays covered. Co-Authored-By: HAL 9000
Let wallet sync enqueue on-chain payment events directly. Co-Authored-By: HAL 9000
Notify users when otherwise unclassified on-chain payments reach the anti-reorg confirmation depth. Use the wallet's effective stored payment so classified funding, splice, close, and sweep transactions stay quiet. Co-Authored-By: HAL 9000
Upstream added a multi-node reserve test after the event commit's original base. Consume its premine events so they do not mask the channel lifecycle assertions after rebasing. Co-Authored-By: HAL 9000
Include the known transaction fee in successful on-chain payment events so consumers do not need to look up the payment record. Clarify the confirmation height and legacy classification documentation too. Co-Authored-By: HAL 9000
Wait for broadcasts before mining so confirmation-depth assertions cannot race the broadcaster. Document why zero-conf splice behavior changes whether the receiving wallet emits an event. Co-Authored-By: HAL 9000
Cover successful on-chain events in the Kotlin full-cycle test and verify that a handled settled event is not re-emitted after a clean restart. Co-Authored-By: HAL 9000
Verify that moving a shallow payment back to unconfirmed does not emit a settled event. Reconfirmation coverage remains with the deferred stale pending-entry fix. Co-Authored-By: HAL 9000
The in-memory store source is shared by the library test utilities and integration-test helpers. Keep its unit test in the library-only module so each integration binary does not register and rerun it. Co-Authored-By: HAL 9000
176ac00 to
c39bdcf
Compare
3 participants
Closes #446.
Based on #432.
In #432 we exposed transactions in store. Here we take a stab at exposing them via events. However, to avoid emitting duplicate events for channel-related transactions, this is in draft until we have lightningdevkit/rust-lightning#3566