Skip to content

fix: hold cells across funding and the watchtower - #1707

Open
LusoCryptoLabs wants to merge 3 commits into
nervosnetwork:developfrom
LusoCryptoLabs:held-cells
Open

LusoCryptoLabs wants to merge 3 commits into
nervosnetwork:developfrom
LusoCryptoLabs:held-cells

Conversation

@LusoCryptoLabs

Copy link
Copy Markdown
Contributor

The watchtower picks the fee cells of its revocation and settlement txs with its own DefaultCellCollector (run_periodic_check). It does not see the cells that pending funding txs reserve (LiveCellsExclusionMap, in CkbChainActor), and funding does not see the cells of the watchtower's txs still in the pool. The indexer lists a cell as live until the block that spends it, so either can pick a cell the other already spent.

Seen on our testnet LSP on 2026-10-28 (a fork with #1623; the code in question is the same on develop): a settlement tx in the pool spent a cell, a tenant's funding tx built nine seconds later put the same cell in, and the funding failed with Resolve failed Unknown(OutPoint ...) once the settlement was committed.

  • ckb::held_cells: one record of held cells per process. Each CkbChainActor publishes its funding inputs there after every change to its exclusion map. The watchtower holds the inputs of each revocation and settlement it sends, for 30 minutes.
  • The funding builder and the watchtower lock every held cell in their collectors before they pick.

A process-wide record was the smallest change that reaches the watchtower's thread. If you would rather have the watchtower ask CkbChainActor for the exclusions, or another shape, we can change it.

We run this on our testnet LSP since 2026-10-29.

Checked on this branch:

  • two tests in ckb::held_cells: an opening's inputs stay locked until it lets them go, and other holders' stay; the watchtower's are locked for their hold and free after it
  • cargo fmt, typos, cargo shear, cargo clippy -p fnn -p fiber-bin --all-targets --all-features -- -D warnings
  • cargo test -p fnn --features rocksdb: all pass except fiber::tests::mpp::test_mpp_waiting_ack_settlement_hands_off_to_durable_retry_queue, which fails on develop too (3 of 3 at 4777d5d)

The watchtower picked fee cells without the cells reserved by pending
funding txs, and funding without the cells of the watchtower's txs in
the pool. The indexer lists a cell as live until its spending block, so
either could pick a cell the other had spent. Keep both in one record
of held cells and lock them in each collector.
The watchtower's hold and its test only exist with the watchtower feature; the wasm build failed clippy on them as dead code.
@LusoCryptoLabs

Copy link
Copy Markdown
Contributor Author

CI: the one failing test, fiber::tests::mpp::test_mpp_waiting_ack_settlement_hands_off_to_durable_retry_queue, also fails on develop since 2026-10-26 (run 40264298846 at 4777d5d, the same panic at test_utils.rs:2213). The other 1463 tests pass on this branch.

@quake
quake requested review from jjyr and quake October 29, 2026 10:08
@gpBlockchain

Copy link
Copy Markdown
Contributor

Security Review

Result: 2 confirmed Medium findings in the held-cell lifecycle.
Reviewed range: 4777d5d64b8cd636c7b08850165e67bbc83690be..93f024cb0a937ce27f4bc6fbde46c41341c70fde
Review model: GPT-6

Validation:

  • Existing broad tests were not run locally; CI is expected to cover them.
  • Reviewed the changed files and related collector and watchtower paths by local source trace; no live chain transaction was submitted.

Notes:

  • Please ensure cell reservations remain consistent when funding and watchtower collectors overlap, and that pending watchtower reservations are restored after a node restart. These are limits of the proposed fix for the existing conflict. Coordinate with the security reviewers for the full evidence and focused regression cases before merge.

@LusoCryptoLabs

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Both hold, as I read the code.

  1. Overlap: the watchtower holds a tx's cells only after the tx is sent, and the chain actor publishes its funding inputs only after the build returns. In between, either side can pick a cell the other has picked but not yet held. Plan: hold a cell when it is picked, under the same lock that filters the held ones, and release the watchtower's hold when its send fails.
  2. Restart: the holds live in memory, so a restart drops them. Plan: before a picked cell is used, ask the node with get_live_cell and include_tx_pool = true, and skip a cell that a tx in the pool already spends. That covers our own txs from before a restart without a store, and also a tx sent by another process with the same key.

If you can share the evidence and the regression cases you mentioned, I'll build the tests from them. Otherwise I'll write one test per finding, each failing on the current branch first.

The watchtower held a tx's cells only after sending it, and an opening
published its inputs only after the build, so either could pick a cell
the other had just picked. Both now pick through TakingCollector, which
holds a cell in the same step that checks it is free, and the
watchtower lets its cells go when a send fails.

The holds live in memory. Each picked cell is also checked with
get_live_cell and include_tx_pool, so a cell a pending tx spends is
skipped after a restart too.
@LusoCryptoLabs

Copy link
Copy Markdown
Contributor Author

Pushed 28456f7 for both findings.

  1. Overlap: openings and the watchtower now pick through TakingCollector. It holds a cell in the same step, under the held-cells lock, that checks no one else holds it, so a cell is held from the moment it is picked. The watchtower's pick is held for a minute while it builds and sends, then for WATCHTOWER_HOLD_MS once sent, and released when the send fails.
  2. Restart: each picked cell is also checked with get_live_cell and include_tx_pool = true, and a cell a pending tx spends is skipped. On a dev chain (ckb 0.210), the input of a pending transfer answered live without the flag and unknown with it, while an untouched cell answered live both ways.

Tests in ckb::held_cells: an opening's pick refused to the watchtower before it is published, the watchtower's pick refused to an opening until released, a pool-spent cell refused with no hold, and a refused cell replaced until the capacity is met. Each new test fails when the part it covers is removed (the funding hold, the watchtower hold, the pool check). The tests share the process-wide holds, so they now run one at a time.

Locally make clippy is clean, and cargo test -p fnn --features rocksdb --lib passes except test_mpp_waiting_ack_settlement_hands_off_to_durable_retry_queue, which fails on this branch's base 4777d5d and passes on develop since 704a48c. I can rebase onto develop if you prefer.

CI is red here for a reason outside this PR: ubuntu-latest moved to ubuntu-26.04 between develop's last green run (2026-10-29, ubuntu-24.04) and this one, and ckb-librocksdb-sys 8.5.4 no longer builds there ('uint64_t' has not been declared in blob_file_meta.h, <cstdint> not included). Every job that builds rocksdb fails the same way, so the next develop push should too. Pinning runs-on: ubuntu-24.04 should bring it back; I can open a PR for that if you want.

If your evidence covers a case these tests miss, I'll add it.

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.

2 participants