Conversation
All Steven's changes in a single patch. Taken from - repo: github.com/stevenroose/rust-bitcoin - commit: 639d552 Did minor changes to reduce the size of the patch, notably removed the `opcodes::all` re-export.
Pull Request Test Coverage Report for Build 8338808138Details
💛 - Coveralls |
| } else { | ||
| Some(LockTime::from(Height::from(low16))) | ||
| } | ||
| } |
There was a problem hiding this comment.
This one we already have (for u32s) in #2549.
We could add a wrapper function for i64s since that's what you get from scriptnums, but I think it might make more sense to just directly add the ability to parse locktimes from Script.
There was a problem hiding this comment.
I think it might make more sense to just directly add the ability to parse locktimes from Script.
Yes, please!
| }; | ||
|
|
||
| /// The default maximum size of scriptints. | ||
| const DEFAULT_MAX_SCRIPTINT_SIZE: usize = 4; |
There was a problem hiding this comment.
The purpose of this constant is that
- All script opcodes take maximum 4-byte numbers as inputs.
- Except CLTV which takes a maximum 5-byte number.
Personally I think it'd be clearer if we just had a read_scriptint and read_cltv_scriptint method which each passed into a read_scriptint_internal and hardcoded the 4 and 5. It's a minor refactor relative to what's here but I'd find it easier to follow if everything was together and we didn't have a new constant to memorize the value of.
There was a problem hiding this comment.
Also encode the constraints in the types.
| let mut buf = [0u8; 8]; | ||
| let len = write_scriptint(&mut buf, n); | ||
| buf[0..len].to_vec() | ||
| } |
There was a problem hiding this comment.
We already have script::Builder::push_int. Should look into the purpose of this method.
| } | ||
|
|
||
| Ok(builder.into_script()) | ||
| } |
There was a problem hiding this comment.
This is interesting but it makes our existing "asm" output first class. We should maybe think about whether we want to change it (bearing in mind that it's currently heavily used, at least in user display, in e.g. Esplora).
- Do we want to encode decimal numbers at all?
- Do we want to drop the
OP_prefix from opcodes? - Should the
OP_PUSHNUM_xnames be shortened to justOP_x? How about theOP_PUSHBYTES_n? - Should we handle
OP_TRUEas an alias forOP_1?OP_FALSEforOP_0?OP_PUSHBYTES_0forOP_0? etc - Should we allow the old
NOPxnames for CSV and CLTV?
We don't need to answer these here ofc but there will be a long debate on the actual PR that introduces this.
| .map(Script::from_bytes) | ||
| self.last().and_then(|last| { | ||
| // From BIP341: | ||
| // If there are at least two witness elements, and the first byte of |
There was a problem hiding this comment.
This change appears to make the logic inconsistent with the comment.
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
I think these three methods should be a single one which returns some sort of composite structure which describes whether there is a control block, whether there is an annex, whether there is a tapscript, etc.
But should be debated in another PR.
There was a problem hiding this comment.
I wanted this too and also to allow going in the opposite direction: create the struct and then "serialize" it as a witness.
|
Thanks for taking the time to look @apoelstra, I had not looked that closely myself yet even! |
|
I'm not sure what to do with this PR.
The least work for me is (2), but that "someone" probably has to be @apoelstra. If you think the BitVM usecase is just in general important enough Andrew then I'll do (1). FTR my priorities currently are:
Thanks |
|
Sounds good. Agreed that hashes and primitives are higher priority. And similar with rpc stuff. I will maybe work on this then when I have some spare mental capacity. I'm glad that Kix bumped it so that I was able to find it again. |
|
Feel free to re-name it |
tcharding
changed the title
|
@tcharding this was discussed internally in a BitVM Alliance group meeting today. A lot of folks are using this branch in their I want to help migrating these changes to a new PR and going over the review rounds. How do you want to tackle this @tcharding? Should I do a new PR, since this will be the 9th circle of rebasing hell? Related to BitVM/BitVM#23 Footnotes
|
|
@storopoli if you are willing to take this on, please do! I am happy to do review to try to push it forward. I think BitVM is very important ... but I guess, not important enough for me to actively do the work. |
|
Yeah man go for it, that would be great. |
|
@storopoli has got this covered. |
6 participants
All Steven's changes in a single patch.
Taken from
Did minor changes to reduce the size of the patch, notably removed the
opcodes::allre-export.This is draft until after v0.32.0 release when we can start to think merging stuff from this. I'd pull out discreet PRs of course.