From c71ae9ac165406d624766ca53e9b845cf439e9bd Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Sat, 22 Jun 2024 06:20:04 +1000 Subject: [PATCH] Move PushBytes::read_scriptint The `push_bytes` module has a `private` module that exists solely to protect the invariant on the `PushBytes` inner byte slice. There is a `PushBytes` impl block outside the private module for functions that can not and do not violate the length invariant. Recently we move the `read_scriptint` method to be on the `PushBytes` but we put it inside the `private` module, since the method only reads off of the slice it cannot invalidate the invariant and does not need to be inside the `private` module. Move the `read_scriptint` method outside of the `private` module to keep that module as small as possible, helping with its stated aim of being the only place that requires auditing. --- bitcoin/src/blockdata/script/push_bytes.rs | 91 +++++++++++----------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/bitcoin/src/blockdata/script/push_bytes.rs b/bitcoin/src/blockdata/script/push_bytes.rs index 7f65b345e..e0e611626 100644 --- a/bitcoin/src/blockdata/script/push_bytes.rs +++ b/bitcoin/src/blockdata/script/push_bytes.rs @@ -4,6 +4,8 @@ use core::ops::{Deref, DerefMut}; +use crate::script; + #[allow(unused)] use crate::prelude::*; @@ -21,7 +23,6 @@ mod primitive { use super::PushBytesError; #[allow(unused)] use crate::prelude::*; - use crate::script::{scriptint_parse, Error}; #[cfg(any(target_pointer_width = "16", target_pointer_width = "32"))] fn check_limit(_: usize) -> Result<(), PushBytesError> { Ok(()) } @@ -74,50 +75,6 @@ mod primitive { /// Returns the underlying mutbale bytes. pub fn as_mut_bytes(&mut self) -> &mut [u8] { &mut self.0 } - - /// Decodes an integer in script(minimal CScriptNum) format. - /// - /// Notice that this fails on overflow: the result is the same as in - /// bitcoind, that only 4-byte signed-magnitude values may be read as - /// numbers. They can be added or subtracted (and a long time ago, - /// multiplied and divided), and this may result in numbers which - /// can't be written out in 4 bytes or less. This is ok! The number - /// just can't be read as a number again. - /// This is a bit crazy and subtle, but it makes sense: you can load - /// 32-bit numbers and do anything with them, which back when mult/div - /// was allowed, could result in up to a 64-bit number. We don't want - /// overflow since that's surprising --- and we don't want numbers that - /// don't fit in 64 bits (for efficiency on modern processors) so we - /// simply say, anything in excess of 32 bits is no longer a number. - /// This is basically a ranged type implementation. - /// - /// This code is based on the `CScriptNum` constructor in Bitcoin Core (see `script.h`). - pub fn read_scriptint(&self) -> Result { - let last = match self.as_bytes().last() { - Some(last) => last, - None => return Ok(0), - }; - if self.len() > 4 { - return Err(Error::NumericOverflow); - } - // Comment and code copied from Bitcoin Core: - // https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/script/script.h#L247-L262 - // If the most-significant-byte - excluding the sign bit - is zero - // then we're not minimal. Note how this test also rejects the - // negative-zero encoding, 0x80. - if (*last & 0x7f) == 0 { - // One exception: if there's more than one byte and the most - // significant bit of the second-most-significant-byte is set - // it would conflict with the sign bit. An example of this case - // is +-255, which encode to 0xff00 and 0xff80 respectively. - // (big-endian). - if self.len() <= 1 || (self[self.len() - 2] & 0x80) == 0 { - return Err(Error::NonMinimalPush); - } - } - - Ok(scriptint_parse(self.as_bytes())) - } } macro_rules! delegate_index { @@ -335,6 +292,50 @@ impl PushBytes { /// Returns true if the buffer contains zero bytes. pub fn is_empty(&self) -> bool { self.as_bytes().is_empty() } + + /// Decodes an integer in script(minimal CScriptNum) format. + /// + /// Notice that this fails on overflow: the result is the same as in + /// bitcoind, that only 4-byte signed-magnitude values may be read as + /// numbers. They can be added or subtracted (and a long time ago, + /// multiplied and divided), and this may result in numbers which + /// can't be written out in 4 bytes or less. This is ok! The number + /// just can't be read as a number again. + /// This is a bit crazy and subtle, but it makes sense: you can load + /// 32-bit numbers and do anything with them, which back when mult/div + /// was allowed, could result in up to a 64-bit number. We don't want + /// overflow since that's surprising --- and we don't want numbers that + /// don't fit in 64 bits (for efficiency on modern processors) so we + /// simply say, anything in excess of 32 bits is no longer a number. + /// This is basically a ranged type implementation. + /// + /// This code is based on the `CScriptNum` constructor in Bitcoin Core (see `script.h`). + pub fn read_scriptint(&self) -> Result { + let last = match self.as_bytes().last() { + Some(last) => last, + None => return Ok(0), + }; + if self.len() > 4 { + return Err(script::Error::NumericOverflow); + } + // Comment and code copied from Bitcoin Core: + // https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/script/script.h#L247-L262 + // If the most-significant-byte - excluding the sign bit - is zero + // then we're not minimal. Note how this test also rejects the + // negative-zero encoding, 0x80. + if (*last & 0x7f) == 0 { + // One exception: if there's more than one byte and the most + // significant bit of the second-most-significant-byte is set + // it would conflict with the sign bit. An example of this case + // is +-255, which encode to 0xff00 and 0xff80 respectively. + // (big-endian). + if self.len() <= 1 || (self[self.len() - 2] & 0x80) == 0 { + return Err(script::Error::NonMinimalPush); + } + } + + Ok(script::scriptint_parse(self.as_bytes())) + } } impl PushBytesBuf {