Added tests to check the bolt 2 specs for open_channel (Sending Node) #262
Conversation
src/ln/channelmanager.rs
Outdated
| /// | ||
| /// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is | ||
| /// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000. | ||
| pub fn create_channel_with_id(&self, their_network_key: PublicKey, channel_id: [u8; 32], channel_value_satoshis: u64, push_msat: u64, user_id: u64) -> Result<(), APIError> { |
There was a problem hiding this comment.
I really don't think this needs to be pub. In what case would a user ever care about specifying the temporary_channel_id explicitly?
There was a problem hiding this comment.
The only reason for this change was to make testing easier, but the change was reverted.
src/ln/channelmanager.rs
Outdated
| let nodes = create_network(2); | ||
|
|
||
| // BOLT #2 spec: Sending node must ensure temporary_channel_id is unique from any other channel ID with the same peer. | ||
| // Note: currently a collision on a channel id will panic, not creating the channel with an error will be better as the system can still continue to function |
There was a problem hiding this comment.
We expect the rng to work, so panic!()ing if the rng gives us the same 32 bytes that we saw somewhere is is absolutely a cause for panic/shutdown/burn it to the ground. Much, much better to panic in that case and make the user reconsider how broken their hardware/something is than continue in a very unsafe environment.
src/ln/channel.rs
Outdated
| @@ -412,7 +412,7 @@ impl Channel { | |||
| } | |||
|
|
|||
| // Constructors: | |||
| pub fn new_outbound(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface>, their_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel, APIError> { | |||
| pub fn new_outbound(fee_estimator: &FeeEstimator, keys_provider: &Arc<KeysInterface>, their_node_id: PublicKey,channel_id: [u8; 32], channel_value_satoshis: u64, push_msat: u64, user_id: u64, logger: Arc<Logger>, config: &UserConfig) -> Result<Channel, APIError> { | |||
There was a problem hiding this comment.
Instead of passing the 32 bytes down through the call stack here, maybe hook the rng and change the value returned in testing? In general I think we should be moving away from using the thread_rng/rand crate directly as we are trying to design for unknown environment. We already moved away from it a chunk with KeysInterface and further with #260, and this is actually the last place that rand_u832 is called, we should be able to trivially just move the current fuzztarget implementation into fuzz/fuzz_targets/full_stack_target and replace it with KeysInterface::get_session_key (maybe just renaming it to provide a random [u8; 32] and then having a default fn that converts it to a SecretKey).
There was a problem hiding this comment.
Trying to hook the rng to create a duplicate channel id was very difficult as the rng used cannot be reseeded without having the original handle. I tried many different ways and currently the only way I could do it was by using the rng of the fuzztarget feature. Unfortunately, this means the test will only run when you are testing with the fuzztarget feature enabled.
Secondly, I did attempted to move the fuzztarget rng into the KeysInterface of the full_stack_target but currently the chain::keysinterface is used in channel.rs and not the full_stack_target version. I might be missing something completely, but I'm not sure how to proceed? Please let me know what I should change.
There was a problem hiding this comment.
Oops, I wasnt clear, I was suggesting something more like #260 where you remove the RNG call and instead call a keysinterface function which provides 32 random bytes.
There was a problem hiding this comment.
That way you can hook the specific call in a test keys interface, without having any special mode for it.
There was a problem hiding this comment.
I changed it to be similar to #260, the channel id is now generated in the keysinterface.
neonknight64
force-pushed
the
204-audit-bolt2-closing-negotiation
branch
from
cbae82b to
3fc63e5
Compare
|
Whats the status of this? Looks like the whole thing is commented out now? |
neonknight64
force-pushed
the
204-audit-bolt2-closing-negotiation
branch
4 times, most recently
from
28cf9bf to
5500c61
Compare
neonknight64
force-pushed
the
204-audit-bolt2-closing-negotiation
branch
from
5500c61 to
7d50be1
Compare
|
|
||
| fn get_channel_id(&self) -> [u8; 32] { | ||
| let mut channel_id = [0; 32]; | ||
| fill_bytes(&mut channel_id); |
There was a problem hiding this comment.
The old version actually has u64s byte-swapped.
| @@ -80,6 +80,8 @@ pub trait KeysInterface: Send + Sync { | |||
| fn get_channel_keys(&self, inbound: bool) -> ChannelKeys; | |||
| /// Get a secret for construting an onion packet | |||
| fn get_session_key(&self) -> SecretKey; | |||
| /// Get a unique channel id | |||
There was a problem hiding this comment.
This is a bit sparse. Specifically, should probably mention that we will use the funding txid once it exists at least.
I developed tests that checks that open_channel for the sending node conforms to the bolt 2 spec. Where testing would required major changes to the codebase I provided the location in the code where that specific spec is implemented for manual verification. Please let me know what I can change and improve?
Audit of Bolt 2 spec for Sending Node Channel Establishment - open_channel
Source: https://github.com/rust-bitcoin/rust-lightning/blob/26a71926899459305ae2bb6a58828b215c35a547/src/ln/msgs.rs#L173
Comment: Difficult to test as the commitment_seed keeps changing and I am unable to retrieve the seed without making it public.
Comment: Cant find where this is implemented.
Comment: Unsure how to test this
Comment: Unsure how to test this
Comment: Unsure how to test this