WIP: Bolt 11 invoices #9
Conversation
| regex = "0.2" | ||
| bit-vec = "0.4.4" | ||
| nom = "3.2.1" | ||
| error-chain = "0.11.0" |
There was a problem hiding this comment.
Hmm, do you really need all these deps? Seems like a lot just to save a few lines on parsing code. See-also: dep guidelines in README.
There was a problem hiding this comment.
dep guidelines in README.
Thank you for pointing that out, I probably skipped right ahead to the todo's. Getting rid of nom and bit-vec will be easy (nom looked nice at the beginning, but isn't that well documented etc. and not really of much help, I just stuck with it because it was challenging and fun). I can probably live without error-chain/failure too. Loosing regex could be a bit annoying.
| } | ||
| } | ||
|
|
||
| // TODO: better types instead onf byte arrays |
There was a problem hiding this comment.
Just use rust-bitcoin's Script object directly?
There was a problem hiding this comment.
Wouldn't that have quite bad ergonomics? It forces the user to build a complete script and our library checks if it is one of the three supported types, strips away all the boilerplate/OP_CODEs and extracts the hash that the user probably had to begin with. The other way around seems to be a bit cumbersome too: how do I generate the corresponding address from the script (even worse: a to_address() function will have to return a Result because afaik not every instance of Script is encodable as some type of address)?
That's just how I understood Script from the docs, maybe I missed something.
There was a problem hiding this comment.
Ah, oops, sorry, I was just thinking the deserialize -> internal struct direction, not struct -> serialized form direction. Probably time to clean up the rust-bitcoin address stuff so that it supports more types of addresses... @apoelstra ?
| amount, | ||
| timestamp: Utc::now(), | ||
| tagged: vec![], | ||
| signature: Signature::from_der(&Secp256k1::new(), &[0; 65])?, |
There was a problem hiding this comment.
Shouldn't this be from_compact not from_der (I believe this is busted elsewhere in the codebase right now...)
|
Oh, relatedly, some folks had asked that the BOLT 11 implementation be a separate crate that rust-lightning depend on, which I think is reasonable. Simplest would probably be a new repo in the rust-bitcoin org so that it and rust-lightning can be maintained in parallel.
…On March 8, 2018 3:50:08 PM UTC, Matt Corallo ***@***.***> wrote:
TheBlueMatt commented on this pull request.
Thanks for tackling this! If possible, can you cut down on the
dependancy introduction so that things are more readable/pure Rust
instead of a long mess of macro invocations that you have to look up
the meaning of?
> @@ -18,3 +18,12 @@ bitcoin = "0.11"
rust-crypto = "0.2"
rand = "0.4"
secp256k1 = "0.8"
+bech32 = {git = "https://github.com/sgeisler/rust-bech32"} #
rust-bitcoin/rust-bech32#4
+chrono = "0.4"
+regex = "0.2"
+bit-vec = "0.4.4"
+nom = "3.2.1"
+error-chain = "0.11.0"
Hmm, do you really need all these deps? Seems like a lot just to save a
few lines on parsing code. See-also: dep guidelines in README.
> + Description(String),
+ PayeePubKey(PublicKey),
+ DescriptionHash([u8; 32]),
+ ExpiryTime(Duration),
+ MinFinalCltvExpiry(u64),
+ Fallback(Fallback),
+ Route {
+ pubkey: PublicKey,
+ short_channel_id: u64,
+ fee_base_msat: i32,
+ fee_proportional_millionths: i32,
+ cltv_expiry_delta: u16,
+ }
+}
+
+// TODO: better types instead onf byte arrays
Just use rust-bitcoin's Script object directly?
> +}
+
+impl FromStr for Invoice {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self> {
+ let Bech32 {hrp, data} = s.parse()?;
+
+ let (currency, amount) = Invoice::parse_hrp(&hrp)?;
+
+ Ok(Invoice {
+ currency,
+ amount,
+ timestamp: Utc::now(),
+ tagged: vec![],
+ signature: Signature::from_der(&Secp256k1::new(), &[0;
65])?,
Shouldn't this be from_compact not from_der (I believe this is busted
elsewhere in the codebase right now...)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#9 (review)
|
|
An extra repo would be nice, the name would probably be Btw: thank you for taking the time to review my code :) |
|
That sounds great. Happy to open up a new repo on rust-bitcoin for it! |
When I read that I wondered if there are any other communication channels than this repo and the lightning-dev list? What's the best way to stay in touch? |
|
The comment I was referencing was made at https://www.reddit.com/r/Bitcoin/comments/81shlp/lightning_network_still_needs_a_lot_of_work/dv5sti8/ I'm not aware of any other communication methods for rust-bitcoin stuff, but maybe we should make an IRC channel or something. |
|
I've created an own repo for now. I will copy the license and the relevant parts of the README.md of this repo. We can transfer it to rust-bitcoin whenever you want. |
|
I believe you have to initiate the transfer. |
|
Did rust-bitcoin receive a transfer request? The transfer dialog told me that the transfer would happen when the new owner confirms it. But after pressing the confirm button I got the following error message
|
|
Hmm, well I added you to the org so it should work now. |
# This is the 1st commit message: test utils: refactor fail_payment_along_route for mpp # This is the commit message #2: Add MppId field to HTLCSource as a way to correlate mpp payment paths # This is the commit message #3: Implement Readable/Writeable for HashSet To be used in upcoming commits for MPP ID storage # This is the commit message #4: Add MPP ID to pending_outbound_htlcs We'll use this to correlate MPP shards in upcoming commits # This is the commit message #5: Prevent duplicate PaymentSent events by removing all pending outbound payments associated with the same MPP payment after the preimage is received # This is the commit message #6: Add all_paths_failed field to PaymentFailed see field docs for details # This is the commit message #7: Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. # This is the commit message #8: [fuzz] Swap mode on most messages to account for TLV suffix # This is the commit message #9: Add forward-compat due serialization variants of HTLCFailureMsg Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields. # This is the commit message #10: Convert most P2P msg serialization to a new macro with TLV suffixes The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process. # This is the commit message #11: Update docs to specify where process events is called
# This is the 1st commit message: Update fuzz README with latest instructions # This is the commit message #2: Allow multiple monitor_update_failed calls without requiring calls to channel_monitor_updated in between. Found by the fuzzer # This is the commit message #3: Move CounterpartyForwardingInfo from channel to channelmanager CounterpartyForwardingInfo is public (previously exposed with a `pub use`), and used inside of ChannelCounterparty in channelmanager.rs. However, it is defined in channel.rs, away from where it is used. This would be fine, except that the bindings generator is somewhat confused by this - it doesn't currently support interpreting `pub use` as a struct to expose, instead ignoring it. Fixes lightningdevkit/ldk-garbagecollected#44 # This is the commit message #4: test utils: refactor fail_payment_along_route for mpp # This is the commit message #5: Add MppId field to HTLCSource as a way to correlate mpp payment paths # This is the commit message #6: Implement Readable/Writeable for HashSet To be used in upcoming commits for MPP ID storage # This is the commit message #7: Add MPP ID to pending_outbound_htlcs We'll use this to correlate MPP shards in upcoming commits # This is the commit message #8: Prevent duplicate PaymentSent events by removing all pending outbound payments associated with the same MPP payment after the preimage is received # This is the commit message #9: Add all_paths_failed field to PaymentFailed see field docs for details # This is the commit message #10: Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. # This is the commit message #11: [fuzz] Swap mode on most messages to account for TLV suffix # This is the commit message #12: Add forward-compat due serialization variants of HTLCFailureMsg Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields. # This is the commit message #13: Convert most P2P msg serialization to a new macro with TLV suffixes The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process. # This is the commit message #14: Update docs to specify where process events is called
…-panic-gossip Don't panic if we can't sign a gossip message.
This branch implements lightning invoices and their bech32 en- and decoding. I've already defines the data structures, but I will probably change them to use more semantic types. The parser is written with nom and currently under construction. I haven't worked on serialization yet.
I'd be happy if someone more familiar with the lightning protocol could have a look at the data structures.