Added a newtype for script-pushable slice #1190
Conversation
6d1aa6a to
295372c
Compare
There was a problem hiding this comment.
LGTM, since this is the second type of owned/borrow pair we are working on and there is a fair bit going on I'd like to write a thorough integration test. Both for my learning and because I think it would help to make sure we have all the boilerplate correct. Just checking that if I was to do that it is not treading on your toes @Kixunil?
src/blockdata/script.rs
Outdated
| Builder::new() | ||
| .push_opcode(opcodes::all::OP_DUP) | ||
| .push_opcode(opcodes::all::OP_HASH160) | ||
| // The `self` script is 0x00, 0x14, <pubkey_hash> |
There was a problem hiding this comment.
This is stale now, perhaps move it to the new method v0_p2wpkh. I realize my comment is too low level review for the current draft PR but mentioning it because what I wanted to say was I'm pretty sure I wrote this comment originally and now I can't remember why the first two bytes are 0 and 14 so if you do move the comment perhaps do a better job at explaining them than I did :)
There was a problem hiding this comment.
0x00 is witness v0
0x14 is OP_PUSHDATA (20 bytes) which is pushing the <pubkey_hash>
The original did &self[2..] to skip the 0x00 and 0x14 of the p2wpkh and only push_slice the <pubkey_hash> part. (since it's the same hash as P2PKH).
P2WPKH requires the script code preimage to be a P2PKH script for some reason (which is what this function is creating)
|
No problem! I don't enjoy writing tests too much. ;) Also I don't expect the API to change significantly except for adding stuff that's in |
046dc0b to
3ba0d2b
Compare
|
In principle, this is ready (uses the
So feel free to skip if you think it's not suitable for 0.30. |
5686a1c to
a786315
Compare
|
concept ACK. I think the PR could be split up a bit to make review eaiser:
No matter what there'll be a huge commit that introduces the I'm ambivalent about slipping to 0.31 or not. |
|
Yeah, it did occur to me I could split them, I just didn't have the energy to do it at midnight. :) I will look into it. |
|
Ahh August 13th - @Kixunil I see why I couldn't remember what was going on with |
|
All looks pretty sane to me. |
|
Force push just to (ab)use CI, going to address review. |
So far we deserialized hex into `Vec<u8>` at run time. This was mainly in tests where it had negligible performance cost. However moving the computation to compile time has a few benefits: it allows proving the length of the decoded bytes and identifies potential typos before the code goes through LLVM and other compilation machinery which makes feedback faster. This change uses the `hex_lit` crate to move computation to compile time. It is implemented as `const` declarative macro which doesn't blow up compilation time.
Script parsing is composed of several functions which implicitly rely on various properties. Adding a type that restricts the valid values makes local review easier.
`Signature` only supported serialization into `Vec` which required a heap allocation as well as prevented statically proving maximum length. Adding a specialized type that holds a byte array and size solves this. The solution is very similar to `secp256k1::ecdsa::SerializedSignature`. The difference is that serialized signature in this crate contains sighash bytes flag while in `secp256k1` it doesn't.
|
Separated into multiple commits, I'd like to get this in since it looks like we have some time for it. |
|
Happy to review and get this in -- but heads up current version does not compile (it's not a CI network thing) |
The code previously contained undocumented panic when trying to push slice. This change solves it by adding a newtype that guarantees limited length.
|
Heh, looks like an artifact of my history editing. Should be resolved now. |
| } | ||
| } | ||
|
|
||
| macro_rules! delegate_index { |
There was a problem hiding this comment.
In bcd95fa:
I think this macro can be replaced by a suitable generic impl Index<I> for PushBytes where [u8]: Index<I>, Vec: Index<I>::Output = [u8] or something.
Should be done in a followup, I don't want to have to re-review this thing and it's not a big deal anyay :)
There was a problem hiding this comment.
Then safe code could break our invariant:
strut Evil;
impl Index<Evil> for [u8] {
type Output = [u8];
fn index(&self, index: Evil) -> &Self::Output {
static BIG_ARR: [u8; 4294967297] = [0; 4294967297];
&BIG_ARR
}
}
script.push(&PushBytes::empty()[Evil]);| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(not(any(target_pointer_width = "16", target_pointer_width = "32")))] |
There was a problem hiding this comment.
In bcd95fa:
Maybe a comment here saying "we have 64 bits in mind, but even for esoteric sizes, this code is correct, since it's the conservative one that checks for errors".
|
I'm a little bit wary of how many generics this introduces and what the effect might be on complexity or compile time. But ACK for now and we can revisit later on once we have some better API-analysis tooling in place. |
d83739a Clarify the intention of strange condition (Martin Habovstiak) Pull request description: It may not be obvious why the condition in `push_bytes` module checks for negation of 16 and 32 bit architectures rather than 64 bit. This adds a comment about it being conservative. Addresses #1190 (comment) ACKs for top commit: apoelstra: ACK d83739a tcharding: ACK d83739a Tree-SHA512: 8a674b9869c580a007c189b0d9b0a57023b26eff3afab25322d966e0ccaff767d19dd499243d438832972f351e1e7ea4b8e387a5e3c74569816814fea258b20b
|
@Kixunil Can you maybe find where that undocumented panic was? 4GB is so big, I feel like just documenting the panic would have been sufficient. The same code that would have triggered that panic, would have triggered an std-lib panic anyway on 32-bit architectures, right? |
|
In |
The code previously contained undocumented panic when trying to push
slice. This is now solved by making a newtype that guarantees limited
length.
Closes #1186
Closes #1189
This is done on top of unsized script change. I only request light review of the design for now. I'd like to improve it to use a macro for all unsized types.