push-based decoding #2184
push-based decoding #2184
Conversation
Kixunil
added
enhancement
help wanted
1.0
|
I've just learned about |
|
Neat! I am in favor of using cc #1251 where push-based decoding was first discussed. |
|
I'm not sure how much we can feature-gate it if we want to delegate the impls. (Maintaining two different implementations is annoying; though thankfully they shouldn't change much.) If we're going to do that I would limit it to one release or so. Also I don't think we can reasonably feature gate it on Rust version - Note that 1.55 is already >2 years old (September 2021) and 1.63 is in Debian stable. But if we're going to bump it I'd jump to 1.56 which is still 2 years old and supports edition 2021 (we don't have to use it right away but it's nice to have). |
|
Sounds good to me. cc @tcharding is there an existing issue that we should glom onto to discuss MSRV bumps, or should @Kixunil just make a PR to bump to 1.56 and we'll try to get a bunch of ACKs. cc #1722. |
|
Lets PR and push forward! I tried bringing up Rust 1.57 a few months ago but I was too early because it was not yet 2 years old, #1983 I'd like to see 1.56, edition 2021 would make my life a bunch easier. |
|
I'm keen for this PR. I don't have any comments or suggestions other than that I'm afraid. |
That's fine. I was mainly checking that my approach isn't viewed as horribly complicated or something like that. |
|
Updated to use |
|
Rebased and a bit improved but not really ready. Working on encoding now. |
Kixunil
force-pushed
the
push_decode
branch
5 times, most recently
from
14a51b3
to
f125275
Compare
In effort to modularize the crate and possibly improve `async` support this adds a new crate that defines common consensus-encoding traits based on the `push_decode` crate. This also provides a few common implementations/decoders. This is still considered WIP.
Kixunil
force-pushed
the
push_decode
branch
5 times, most recently
from
804193f
to
c41e5b9
Compare
This implements `bitcoin_consensus_encoding::Decode` for `Transaction` and the types used inside it to demonstrate how `push_decode` can be used to abstract over the byte source (sync/async, std/no-std). The change is designed to be incremental so the exisitng code continues to use old traits, we're just adding new capabilities that may later replace the current encoding approach.
|
I believe this is potentially ready for merge. |
|
OK, maybe not entirely. I think you can still review it since the changes will be minor. Will fix tomorrow. |
|
The git history is quite messed up and I'm not sure what would be the best way to modify it for you to understand the changes easily. Does splitting encoding/decoding make sense? Probably there shouldn't be commits "split file" and "add documentation" I just didn't have much energy yesterday to clean it up. |
Kixunil
force-pushed
the
push_decode
branch
4 times, most recently
from
b4bdc82
to
1ddff32
Compare
Kixunil
added
the
C-consensus-encoding
|
I'd basically say, do as much splitting as you can get away (e.g. between push/pull and between encoding/decoding and between traits and implementations) with while still having commits that compile by themselves. But then the commits should put stuff in their final locations with bugfixes applied. |
Yes, I was constantly editing history and then gave up just to get this to compile (still no clue about the embedded failure), then I became unsure what's the best approach, so I took a break and asked. Will do it properly (but first resolve the embedded stuff.) |
|
I started reviewing but its going to take me a few goes. |
| #[cfg(feature = "std")] | ||
| fn consensus_decode<R: std::io::BufRead + ?Sized>(reader: &mut R) -> Result<Self, ReadError<<Self::Decoder as Decoder>::Error>> { | ||
| push_decode::decode_sync_with::<Self::Decoder, _>(reader, Default::default()) | ||
| } |
There was a problem hiding this comment.
This can be on io::BufRead and not feature gated soon as we release io, right? Is that the plan?
There was a problem hiding this comment.
No, I want to remove dependency on IO when we have this ready. I re-implement it in terms of bitcoin-io in bitcoin::consensus to maintain compatibility in transition period.
no_std users can simply use the lgio bridge function in push_decode or write their own bridge (it's just a few LOC).
| } | ||
|
|
||
| pub(crate) use impl_encodable_using_encode; | ||
|
|
There was a problem hiding this comment.
These are just glue, right? The Encodable trait goes eventually once consenus-encoding is released?
There was a problem hiding this comment.
Yes, for compatibility, to be able to upgrade progressively (and thus test the code) without breaking anything.
|
@Kixunil I added |
|
Yeah, didn't have much time to clean this up. |
|
No sweat, when you get to it. |
|
Hello @ksedgwic, I just saw ligtning-signer and thought you might be interested in this PR. |
|
you might be interested in https://gitlab.com/lightning-signer/txoo/-/tree/main/push-decoder?ref_type=heads for comparison edit: oh, @tcharding already linked above |
|
Yeah, saw it. It's super interesting how similar they are. Aside from method naming, I just used traits since the thing repeats and there are convenience methods that can be written generically. |
|
I'd like to review this soon but needs rebase. |
|
Unfortunately I wasn't feeling well lately for these kinds of big changes. I thought it's temporary and was just putting it off but given it's going on for a while, I'm not sure anymore. |
|
Ok, that's definitely understandable. I think that means though that we will need to punt on push_decoding for at least one release cycle (i.e. 3 months), probably two and possibly more. Meanwhile @tcharding and I will continue to work on crate smashing. |
I think that's OK. |
This is a demo of push-based decoding - sloppy programming ahead! Insufficient error handling, no DoS protection, missing checks during decoding, missing documentation, some things (no-std) not tested, different MSRV...This is now ready.
I'm mainly gathering general feedback: is this something we want to do? Do you think the added complexity is worth it? Do you have other ideas to improve it (other than intentionally ignored parts).
Note that this is currently just decoding, encoding to come later unless this proposal is rejected.
Also regarding MSRV, I could make it lower but it'd help if I didn't have to. It should be 1.51 but I didn't actually check - I tested with 1.63. What do you think about bumping it?
Closes #1741
Closes #1251