Onion messages v1 pre-refactor #1518
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1518 +/- ##
==========================================
+ Coverage 90.93% 92.19% +1.26%
==========================================
Files 80 80
Lines 43386 52214 +8828
Branches 43386 52214 +8828
==========================================
+ Hits 39451 48139 +8688
- Misses 3935 4075 +140
Continue to review full report at Codecov.
|
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
2 times, most recently
from
29379ae to
2a58ae4
Compare
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
4 times, most recently
from
97de6f3 to
18c3649
Compare
| // complete. | ||
| fn write_all(&mut self, src: &[u8]) -> Result<(), io::Error> { | ||
| let mut src_idx = 0; | ||
| loop { |
There was a problem hiding this comment.
nit: maybe ideally lets not rely on bytes_written to be 0 (and the write "succeeding") when we get to the end and check it as a loop condition?
There was a problem hiding this comment.
Not sure I understood this, plz check out latest version
There was a problem hiding this comment.
Oops, instead of calling write outside the loop lets just check if src.len() - src_idx is > 0.
There was a problem hiding this comment.
Oops, instead of calling
writeoutside the loop lets just check ifsrc.len() - src_idxis > 0.
Done
| } | ||
|
|
||
| /// Enables simultaneously writing and encrypting a byte stream into a Writer. | ||
| pub(crate) struct ChaChaPolyWriter<'a, W: Writer> { |
There was a problem hiding this comment.
does this need to be pub at all or is it just used by ChaChaPolyWriteAdapter?
There was a problem hiding this comment.
Oops @TheBlueMatt I need to make this pub(crate) again for use in blinded route construction (the Read version can stay private): d74a26b#diff-42d0fc04e68d7ce058f818e06b0dddf39f2d0661f0e86c973a50c0ace454e8b1R182
lightning/src/util/ser_macros.rs
Outdated
| @@ -144,6 +147,9 @@ macro_rules! check_missing_tlv { | |||
| ($last_seen_type: expr, $type: expr, $field: ident, ignorable) => {{ | |||
| // no-op | |||
| }}; | |||
| ($last_seen_type: expr, $type: expr, $field: ident, (read_arg, $read_arg: expr)) => {{ | |||
There was a problem hiding this comment.
read_arg is pretty ambiguous can we say, like optional_explicit_length_with_args? I dunno, that's kinda verbose but read seems redundant.
There was a problem hiding this comment.
Fine with that. @jkczyz thoughts?
There was a problem hiding this comment.
Hmm... maybe we can make this less of a special case and instead as a variation of option that gives the trait as an ident macro param. Something like:
decode_tlv_stream!(&writer.0[..], {
(1, read_adapter, option: (LengthReadableArgs, arg: rho)),
});Syntax-wise, I think I suggested awhile ago we changed (default_value, $default: expr) to (default: $default: expr), which this would align better with. I guess the exact syntax isn't too important now.
There was a problem hiding this comment.
Maybe even drop the arg: token such that non-Args traits could be used without special casing. Don't have a strong opinion on that part.
There was a problem hiding this comment.
I had to move the ( outside of the option to work with the tt type in decode_tlv_stream, but this seems to work.
diff --git a/lightning/src/util/chacha20poly1305rfc.rs b/lightning/src/util/chacha20poly1305rfc.rs
index 3b2d2627..5a8727cc 100644
--- a/lightning/src/util/chacha20poly1305rfc.rs
+++ b/lightning/src/util/chacha20poly1305rfc.rs
@@ -393,7 +393,7 @@ mod tests {
// Now deserialize the object back and make sure it matches the original.
let mut read_adapter: Option<ChaChaPolyReadAdapter<TestWriteable>> = None;
decode_tlv_stream!(&writer.0[..], {
- (1, read_adapter, (read_arg, rho)),
+ (1, read_adapter, (option: LengthReadableArgs, rho)),
});
assert_eq!(writeable, read_adapter.unwrap().readable);
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 35e4ecf6..816426b1 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -118,7 +118,7 @@ macro_rules! check_tlv_order {
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, ignorable) => {{
// no-op
}};
- ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (read_arg, $read_arg: expr)) => {{
+ ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
// no-op
}};
}
@@ -147,7 +147,7 @@ macro_rules! check_missing_tlv {
($last_seen_type: expr, $type: expr, $field: ident, ignorable) => {{
// no-op
}};
- ($last_seen_type: expr, $type: expr, $field: ident, (read_arg, $read_arg: expr)) => {{
+ ($last_seen_type: expr, $type: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
// no-op
}};
}
@@ -169,9 +169,8 @@ macro_rules! decode_tlv {
($reader: expr, $field: ident, ignorable) => {{
$field = ser::MaybeReadable::read(&mut $reader)?;
}};
- // Read a ser::LengthReadableArgs
- ($reader: expr, $field: ident, (read_arg, $read_arg: expr)) => {{
- $field = Some(ser::LengthReadableArgs::read(&mut $reader, $read_arg)?);
+ ($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
+ $field = Some($trait::read(&mut $reader $(, $read_arg)*)?);
}};
}There was a problem hiding this comment.
Thanks Jeff, added that! Sorry, I forgot to add a fixup commit :(
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
18c3649 to
9bf811c
Compare
| let mut write_buffer = [0; 8192]; | ||
| let mut bytes_written = (&mut write_buffer[..]).write(&src[src_idx..]).expect("In-memory writes can't fail"); | ||
| while bytes_written != 0 { |
There was a problem hiding this comment.
nit: Could make this a loop with a break and move the statements inside to avoid some duplication.
There was a problem hiding this comment.
Let me know what you think of the latest version. I implemented it per Matt's previous comment: #1518 (comment). FYI the first version did use a loop and a break.
| check_object_read_write!(small_writeable); | ||
| } | ||
|
|
||
| fn do_chacha_stream_adapters_ser_macros() -> Result<(), DecodeError> { |
There was a problem hiding this comment.
Can this be inlined in the test?
There was a problem hiding this comment.
The problem is the ser macros use ?, which can only be used in a method that returns a Result. Would you prefer a different way?
There was a problem hiding this comment.
Ah, I see. Nah, this is fine as is then.
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
9bf811c to
d9de986
Compare
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
d9de986 to
394bb26
Compare
To get the next hop's packet's pubkey. This will be used to DRY onion message forwarding in the upcoming Onion Messages PR lightningdevkit#1503
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
394bb26 to
558aa1d
Compare
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
558aa1d to
6bc313e
Compare
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
6bc313e to
a7dbec1
Compare
|
Sorry had to force push again, I was missing one change I thought I added(?) [making ChaChaPolyWriter Edit: never mind, it doesn't need to be pub(crate), removed |
In the upcoming onion messages PR, this will allow us to avoid encoding onion message encrypted data into an intermediate Vec before encrypting it. Instead we encode and encrypt at the same time using this new ChaChaPolyWriteAdapter object.
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
a7dbec1 to
707db54
Compare
In the upcoming onion messages PR, this will allow us to avoid decrypting onion message encrypted data in an intermediate Vec before decoding it. Instead we decrypt and decode it at the same time using this new ChaChaPolyReadAdapter object. In doing so, we need to adapt the decode_tlv_stream macro such that it will decode a LengthReadableArgs, which is a new trait as well. This trait is necessary because ChaChaPoly needs to know the total length ahead of time to separate out the tag at the end.
valentinewallace
force-pushed
the
2022-06-OMs-prefactor
branch
from
707db54 to
ee805e9
Compare
Pre-factor for #1503. See individual commit messages.