Script improvements #4089
Merged
Script improvements #4089
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The newtype sanity rules (a name I came up with): * Newtypes should have at most one constructor that directly references the inner field. * Newtypes should have at most three accessor methods that directly reference the ineer field: one for owned access, the second for borrowed and the third for mutably borrowed. * All other methods should use the methods above to perform operations on the newtype and not directly access the fields. This commit makes `Script` and `ScriptBuf` obey these except for `reserve` and `reserve_exact` since we don't have `as_mut_vec` method. As a side effect it also adds `const` to `ScriptBuf::from_bytes`.
We're confident these methods will never perform syscalls, so it's fine to add `const` to them.
We have several trait implementations for `Script` and `ScriptBuf` in a common module so that it's easy to verify that they are same but `Deref` and `DerefMut` should *not* be implemented for `Script` so having them in the common module is not helpful. This moves them to the appropriate `Owned` module.
The `Arbitrary` trait allows zero-copy implementations for borrowed types, so this adds the implementation for borrowed `Script`.
These methods are either newtype casts that should compile to no-ops or directly calling into some other function with at most some pointer adjustments. As such making them `#[inline]` is definitely benefitial. There are also methods that check length and then call some other function. These are also worth inlining since the length could be known at compile time and the check could be eliminated.
These comments said that the modules should be private but they already are. Also, the internals of the newtypes became private a few commits ago.
There are already several methods referring to capacity but none to retrieve it. Those methods also promise certain behavior that mandates having *a* capacity field inside the struct, so no changes in layout will ever remove it. Thus it's OK to expose the field. Aside from exposing the field, this also fixes up the tests to obey the sanity rules.
The Electrum protocol uses hashes of `script_pubkey` that might look similar to the ones we have in the crate and could be confused. This change notes that to hopefully avoid the confusion. Resolves rust-bitcoin#3997
These were re-implementing hashing after the check rather than calling the `_unchecked` method, so this replaces the manual implementation with the method.
Pull Request Test Coverage Report for Build 13442187736Details
💛 - Coveralls |
Using `Self` instead of specific type name can make some refactorings easier.
cd7cda8 to
5680b4e
Compare
tcharding
approved these changes
Feb 20, 2025
Comment on lines
+38
to
+39
| /// Note: there is another "script hash" object in bitcoin ecosystem (Electrum protocol) that | ||
| /// uses 256-bit hash and hashes a semantically different script. Thus, this type cannot |
There was a problem hiding this comment.
Suggested change
| /// Note: there is another "script hash" object in bitcoin ecosystem (Electrum protocol) that | |
| /// uses 256-bit hash and hashes a semantically different script. Thus, this type cannot | |
| /// Note: there is another "script hash" object in the bitcoin ecosystem (Electrum protocol) | |
| /// that uses 256-bit hash and hashes a semantically different script. Thus, this type cannot |
Adds 'the' in front of 'bitcoin eccosystem and maintains 100 column width.
There was a problem hiding this comment.
I suggest we do these later unless I have to push. This PR blocks script tagging and while I don't have it ready yet, I'd like to parallelize our work.
Comment on lines
+45
to
+46
| /// Note: there is another "script hash" object in bitcoin ecosystem (Electrum protocol) that | ||
| /// looks similar to this one also being SHA256, however, they hash semantically different |
There was a problem hiding this comment.
Suggested change
| /// Note: there is another "script hash" object in bitcoin ecosystem (Electrum protocol) that | |
| /// looks similar to this one also being SHA256, however, they hash semantically different | |
| /// Note: there is another "script hash" object in the bitcoin ecosystem (Electrum protocol) | |
| /// that looks similar to this one also being SHA256, however, they hash semantically different |
|
This is a sexy PR. |
|
|
||
| /// Returns the number of **bytes** available for writing without reallocation. | ||
| /// | ||
| /// It is guaranteed that `script.capacity() >= script.len()` always holds. |
There was a problem hiding this comment.
is script.capacity() >= script.len() an invariant here?
There was a problem hiding this comment.
Of course, same as on Vec.
I did briefly think about supporting capacity = 0 for 'static references but that has its downsides and scripts should never be static except for the empty one in script_sig which doesn't allocate anyway. (Also we could still do it by pretending that capacity == len whenever it's 0.)
This was referenced Feb 23, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements various improvements related to
Script. Please refer to the individual commits for details.This is a part of #4059