Skip to content

smite-ir: generate valid channel types#57

Open
morehouse wants to merge 1 commit intomasterfrom
ir_channel_type
Open

smite-ir: generate valid channel types#57
morehouse wants to merge 1 commit intomasterfrom
ir_channel_type

Conversation

@morehouse
Copy link
Copy Markdown
Owner

Helps the fuzzer choose valid channel types so that deeper paths can be reached in the open_channel->accept_channel flow.

Ref: #5 (Milestone 1)

Copy link
Copy Markdown
Contributor

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@NishantBansal2003 NishantBansal2003 left a comment

Choose a reason for hiding this comment

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

Looks fine, I only have a few nits

Also, if you go ahead with those nits, I think the enum variant names should be updated accordingly based on the comments

Comment thread smite-ir/src/operation.rs Outdated
/// - `option_anchors` (bits 22 and 12)
/// - `zero_fee_commitments` (bit 40)
/// - `option_simple_taproot` (bit 80)
/// - `option_simple_taproot` staging (bit 180)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - `option_simple_taproot` staging (bit 180)
/// - `option_simple_taproot_staging` (bit 180)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

Comment thread smite-ir/src/operation.rs Outdated
Comment on lines +197 to +201
// 22 = option_anchors_zero_fee_htlc_tx
// 40 = option_anchor_zero_fee_commitments
// 46 = option_scid_alias
// 50 = option_zeroconf
// 80 = option_simple_taproot_final
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: would be great if we stay consistent with what the spec says:

Suggested change
// 22 = option_anchors_zero_fee_htlc_tx
// 40 = option_anchor_zero_fee_commitments
// 46 = option_scid_alias
// 50 = option_zeroconf
// 80 = option_simple_taproot_final
// 22 = option_anchors
// 40 = zero_fee_commitments
// 46 = option_scid_alias
// 50 = option_zeroconf
// 80 = option_simple_taproot

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Thanks, good catch. Done, and updated enum variants.

Comment thread smite-ir/src/tests.rs
Comment on lines +312 to +313
}
#[test]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
#[test]
}
#[test]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

@NishantBansal2003
Copy link
Copy Markdown
Contributor

One more thing -- how about adding cases where mutually exclusive channel types are set, so we can verify that implementations reject it? for eg option_anchors with option_simple_taproot

Helps the fuzzer choose valid channel types so that deeper paths can be
reached in the open_channel->accept_channel flow.
@morehouse
Copy link
Copy Markdown
Owner Author

One more thing -- how about adding cases where mutually exclusive channel types are set, so we can verify that implementations reject it? for eg option_anchors with option_simple_taproot

I'm not sure about this. The idea was to help the fuzzer generate correct channel types, and adding lots of invalid combinations works against that.

In order to verify rejection of incorrect types, we also need a special fuzz target that verifies the rejection occurs. If we're going to do that, we could probably hard-code all the other open_channel fields to be valid and then just use fuzz input against channel_type. That would be a much more effective targeted test.

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.

3 participants