Skip to content

smite-ir: implement funding flow generator#141

Draft
NishantBansal2003 wants to merge 6 commits into
morehouse:masterfrom
NishantBansal2003:funding-flow-generator
Draft

smite-ir: implement funding flow generator#141
NishantBansal2003 wants to merge 6 commits into
morehouse:masterfrom
NishantBansal2003:funding-flow-generator

Conversation

@NishantBansal2003

Copy link
Copy Markdown
Contributor

Adds funding_created and channel_ready generators.
Depends-on: #137

Verified that all targets correctly send funding_signed and channel_ready once the commitment transaction is signed and the funding transaction confirms. For CLN, we need to reduce the blockchain polling interval to receive channel_ready promptly. We should also bump the CLN version, since I observed crashes that have been fixed in its latest release.

All other targets were able to reach channel_ready without any issues, but for CLN I had to (mentioning this so we can discuss a solution and add a fix in smite):

  • patch the CLN polling interval from 30 seconds to 2 seconds (I arbitrarily chose the same value as LDK)
  • patch recv_non_ping to also reject gossip messages, since CLN was apparently sending channel_update before sending channel_ready

cln-coverage.tar.gz
eclair-coverage.tar.gz
ldk-coverage.tar.gz
lnd-coverage.tar.gz

Prevent funding transactions from reusing the same
inputs across multiple transaction constructions. Without
this, independently built funding transactions can select
overlapping UTXOs, causing later broadcasts to fail as
non-fee-bumping RBF replacements or making signing
fail once the shared inputs have already been spent.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
The fuzzer may broadcast a transaction that is already
confirmed. Its inputs are spent, so the wallet can no
longer fully sign it. Check the confirmation count before
signing and return early when the transaction is already
on-chain.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Pass a maxfeerate of 0 to bitcoin-cli sendrawtransaction
so broadcasts are accepted at any fee rate, bypassing
the default high-feerate safety cap.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Reject funding transactions that would not be accepted
under Bitcoin's default relay policy. Funding construction
now fails for feerates below the minimum relay feerate and
funding outputs below the dust threshold, avoiding
transactions that cannot be relayed or mined by default.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

@morehouse morehouse left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I'm impressed at the coverage achieved already with disjoint generators for each message exchange. Do you still think it would we be worth creating a combined generator that threads all the correct variables through the whole flow?

We should also bump the CLN version, since I observed crashes that have been fixed in its latest release.

I have a PR to bump versions for CLN and LND. Once I've finished testing against LND I'll upload the PR.

patch the CLN polling interval from 30 seconds to 2 seconds (I arbitrarily chose the same value as LDK)

Yes, let's do this. If you've already verified that 2 seconds works, let's do that right now and tune it down after further experiments.

patch recv_non_ping to also reject gossip messages, since CLN was apparently sending channel_update before sending channel_ready

Yes I think in general we want to skip gossip when receiving messages, since peers are free to send gossip at any time. We can revisit the strategy here if/when we actually want to extract fields from the gossip.

Comment on lines +25 to +26
let second_per_commitment_point = builder.generate_fresh(VariableType::Point, rng);
let short_channel_id = builder.generate_fresh(VariableType::ShortChannelId, rng);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are second_per_commitment_point and short_channel_id generated fresh instead of doing pick_variable?

Comment thread smite-ir/src/tests.rs
Comment on lines +1209 to +1218
// At least 1 DerivePoint instructions.
let derive_count = program
.instructions
.iter()
.filter(|i| matches!(i.operation, Operation::DerivePoint))
.count();
assert!(
derive_count >= 1,
"expected at least one DerivePoint, got {derive_count}"
);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems odd. I think it's true, there will always be at least one DerivePoint, but is it really a fundamental part of the structure that we want to verify?

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