← Watch feed
Replace `bool` in `Encoder` with an enum
Public commit record
68/100 · Adequate
What the developer wrote
Authored by Martin Habovstiak
Replace `bool` in `Encoder` with an enum
It was difficult to remember the meaning of `true` and `false` returned
from `advance` making it harder to write and read the code. The
idiomatic solution to such problem is to replace `bool` with an enum
which this commit does.
To make it less annoying, the enum defines methods that can convert it
to bool readably. Notably, they are not named `is_` but `has_finished`
and `has_more` which reads better in the code that's dealing with them.
E.g. `if encoder.advance().has_finished()` reads naturally.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit is a straightforward code cleanup: it replaces a true/false return value from an encoder's 'advance' method with a clearly-named enum (HasMore/Finished). The behavior of the code is unchanged; it only becomes easier to read and maintain. There is no security fix or vulnerability here.
Recommended action
No security action required. Treat as a normal maintainability refactor during code review.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch refactors the Encoder::advance method across the rust-bitcoin codebase to return a new EncoderStatus enum instead of a bare bool. All call sites are updated to use has_more()/has_finished() helpers. The logic is preserved one-to-one: previous true returns become EncoderStatus::HasMore, previous false returns become EncoderStatus::Finished, and conditions like if !encoder.advance() become if encoder.advance().has_finished(). No functional changes are introduced.
Changed components
bitcoin-consensus-encodingbitcoin-hashesbitcoin-iobitcoin-p2pbitcoin-primitivesfuzz testsInspect captured patch +106 / −69
diff --git a/consensus_encoding/src/compact_size.rs b/consensus_encoding/src/compact_size.rs
index 50585d2b..9076afdb 100644
--- a/consensus_encoding/src/compact_size.rs
+++ b/consensus_encoding/src/compact_size.rs
@@ -9,7 +9,7 @@
use internals::array_vec::ArrayVec;
use crate::decode::Decoder;
-use crate::encode::{Encoder, ExactSizeEncoder};
+use crate::encode::{Encoder, EncoderStatus, ExactSizeEncoder};
use crate::error::{
CompactSizeDecoderError, CompactSizeDecoderErrorInner, LengthPrefixExceedsMaxError,
};
@@ -115,8 +115,8 @@ impl Encoder for CompactSizeEncoder {
fn current_chunk(&self) -> &[u8] { &self.buf }
#[inline]
- fn advance(&mut self) -> bool {
- false
+ fn advance(&mut self) -> EncoderStatus {
+ EncoderStatus::Finished
}
}
diff --git a/consensus_encoding/src/encode/encoders.rs b/consensus_encoding/src/encode/encoders.rs
index d35be7c3..c5b9a615 100644
--- a/consensus_encoding/src/encode/encoders.rs
+++ b/consensus_encoding/src/encode/encoders.rs
@@ -12,7 +12,7 @@
use core::fmt;
-use super::{Encode, Encoder, ExactSizeEncoder};
+use super::{Encode, Encoder, EncoderStatus, ExactSizeEncoder};
/// An encoder for a single byte slice.
#[derive(Debug, Clone)]
@@ -28,7 +28,7 @@ impl<'sl> BytesEncoder<'sl> {
impl Encoder for BytesEncoder<'_> {
fn current_chunk(&self) -> &[u8] { self.sl }
- fn advance(&mut self) -> bool { false }
+ fn advance(&mut self) -> EncoderStatus { EncoderStatus::Finished }
}
impl<'sl> ExactSizeEncoder for BytesEncoder<'sl> {
@@ -52,7 +52,7 @@ impl<const N: usize> Encoder for ArrayEncoder<N> {
fn current_chunk(&self) -> &[u8] { &self.arr }
#[inline]
- fn advance(&mut self) -> bool { false }
+ fn advance(&mut self) -> EncoderStatus { EncoderStatus::Finished }
}
impl<const N: usize> ExactSizeEncoder for ArrayEncoder<N> {
@@ -79,7 +79,7 @@ impl<const N: usize> Encoder for ArrayRefEncoder<'_, N> {
fn current_chunk(&self) -> &[u8] { self.arr }
#[inline]
- fn advance(&mut self) -> bool { false }
+ fn advance(&mut self) -> EncoderStatus { EncoderStatus::Finished }
}
impl<const N: usize> ExactSizeEncoder for ArrayRefEncoder<'_, N> {
@@ -136,16 +136,16 @@ impl<T: Encode> Encoder for SliceEncoder<'_, T> {
self.cur_enc.as_ref().map(T::Encoder::current_chunk).unwrap_or_default()
}
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
let Some(cur) = self.cur_enc.as_mut() else {
- return false;
+ return EncoderStatus::Finished;
};
loop {
// On subsequent calls, attempt to advance the current encoder and return
// success if this succeeds.
- if cur.advance() {
- return true;
+ if cur.advance().has_more() {
+ return EncoderStatus::HasMore;
}
// self.sl guaranteed to be non-empty if cur is non-None.
self.sl = &self.sl[1..];
@@ -154,11 +154,11 @@ impl<T: Encode> Encoder for SliceEncoder<'_, T> {
if let Some(x) = self.sl.first() {
*cur = x.encoder();
if !cur.current_chunk().is_empty() {
- return true;
+ return EncoderStatus::HasMore;
}
} else {
self.cur_enc = None; // shortcut the next call to advance()
- return false;
+ return EncoderStatus::Finished;
}
}
}
@@ -195,7 +195,7 @@ macro_rules! define_encoder_n {
}
#[inline]
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
match self.cur_idx {
$(
$enc_idx => {
@@ -203,14 +203,14 @@ macro_rules! define_encoder_n {
if $enc_idx == $idx_limit - 1 {
return self.$enc_field.advance()
}
- // For all others, return true, or increment to next encoder
- if !self.$enc_field.advance() {
+ // For all others, return EncoderStatus::HasMore, or increment to next encoder
+ if self.$enc_field.advance().has_finished() {
self.cur_idx += 1;
}
- true
+ EncoderStatus::HasMore
}
)*
- _ => false,
+ _ => EncoderStatus::Finished,
}
}
}
diff --git a/consensus_encoding/src/encode/mod.rs b/consensus_encoding/src/encode/mod.rs
index c8e33220..750fdafa 100644
--- a/consensus_encoding/src/encode/mod.rs
+++ b/consensus_encoding/src/encode/mod.rs
@@ -96,7 +96,39 @@ pub trait Encoder {
/// in such state is a bug (but not UB) unless the specific encoder documents otherwise. While
/// usually the encoder simply stays in the last possible state this MUST NOT be relied upon by
/// the callers.
- fn advance(&mut self) -> bool;
+ fn advance(&mut self) -> EncoderStatus;
+}
+
+/// Indicates whether the encoder still has bytes available or it is finished.
+///
+/// This is returned from the [`Encoder::advance`] method to indicate whether encoding should stop
+/// or continue.
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+#[must_use = "encoding has to stop when Finished is returned"]
+pub enum EncoderStatus {
+ /// The encoder has more bytes available (not yet finished).
+ ///
+ /// The [`current_chunk`](Encoder::current_chunk) method should be called to obtain them and
+ /// write them out after which [`advance`](Encoder::advance) should be called again to obtain
+ /// the next chunk (if any).
+ HasMore,
+
+ /// The encoding has ended, no more bytes are available.
+ ///
+ /// No encoder methods (other than drop) may be called after this variant is returned.
+ Finished,
+}
+
+impl EncoderStatus {
+ /// Returns `true` if `self` is `HasMore`, `false` otherwise.
+ pub fn has_more(&self) -> bool {
+ matches!(self, EncoderStatus::HasMore)
+ }
+
+ /// Returns `true` if `self` is `Finished`, `false` otherwise.
+ pub fn has_finished(&self) -> bool {
+ matches!(self, EncoderStatus::Finished)
+ }
}
/// Implements a newtype around an encoder.
@@ -137,7 +169,7 @@ macro_rules! encoder_newtype {
fn current_chunk(&self) -> &[u8] { self.0.current_chunk() }
#[inline]
- fn advance(&mut self) -> bool { self.0.advance() }
+ fn advance(&mut self) -> $crate::EncoderStatus { self.0.advance() }
}
}
}
@@ -205,7 +237,7 @@ impl<T: Encoder> EncoderByteIter<T> {
&self.enc.current_chunk()[self.position..]
} else {
loop {
- if !self.enc.advance() {
+ if self.enc.advance().has_finished() {
return &[];
}
if !self.enc.current_chunk().is_empty() {
@@ -227,7 +259,7 @@ impl<T: Encoder> Iterator for EncoderByteIter<T> {
// overflow.
self.position += 1;
return Some(*b);
- } else if !self.enc.advance() {
+ } else if self.enc.advance().has_finished() {
return None;
}
self.position = 0;
@@ -246,7 +278,7 @@ impl<T: Encoder> Iterator for EncoderByteIter<T> {
return Some(*b);
}
n -= self.enc.current_chunk().len() - self.position;
- if !self.enc.advance() {
+ if self.enc.advance().has_finished() {
return None;
}
loop {
@@ -255,7 +287,7 @@ impl<T: Encoder> Iterator for EncoderByteIter<T> {
return Some(*b);
}
n -= self.enc.current_chunk().len();
- if !self.enc.advance() {
+ if self.enc.advance().has_finished() {
return None;
}
}
@@ -301,7 +333,7 @@ where
let mut vec = Vec::new();
loop {
vec.extend_from_slice(encoder.current_chunk());
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
@@ -344,7 +376,7 @@ where
{
loop {
writer.write_all(encoder.current_chunk())?;
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
@@ -397,7 +429,7 @@ pub fn check_encoder<T: Encoder + ?Sized>(encoder: &mut T, mut expected: &[u8])
bytes_processed += chunk.len();
expected = &expected[chunk.len()..];
chunk_number += 1;
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
@@ -412,5 +444,10 @@ impl<T: Encoder> Encoder for Option<T> {
}
}
- fn advance(&mut self) -> bool { self.as_mut().is_some_and(Encoder::advance) }
+ fn advance(&mut self) -> EncoderStatus {
+ match self {
+ Some(encoder) => encoder.advance(),
+ None => EncoderStatus::Finished,
+ }
+ }
}
diff --git a/consensus_encoding/src/lib.rs b/consensus_encoding/src/lib.rs
index d9d08f75..90ff151a 100644
--- a/consensus_encoding/src/lib.rs
+++ b/consensus_encoding/src/lib.rs
@@ -96,7 +96,7 @@ pub use self::encode::{drain_to_vec, encode_to_vec};
#[doc(inline)]
pub use self::encode::{drain_to_writer, encode_to_writer};
#[doc(inline)]
-pub use self::encode::{check_encode, check_encoder, Encode, Encoder, EncoderByteIter, ExactSizeEncoder};
+pub use self::encode::{check_encode, check_encoder, Encode, Encoder, EncoderStatus, EncoderByteIter, ExactSizeEncoder};
#[cfg(feature = "alloc")]
#[doc(no_inline)]
pub use self::error::LengthPrefixExceedsMaxError;
diff --git a/consensus_encoding/tests/encode.rs b/consensus_encoding/tests/encode.rs
index eb213a10..c91a3256 100644
--- a/consensus_encoding/tests/encode.rs
+++ b/consensus_encoding/tests/encode.rs
@@ -6,8 +6,8 @@
use std::io::{Cursor, Write};
use bitcoin_consensus_encoding::{
- ArrayEncoder, ArrayRefEncoder, BytesEncoder, check_encoder, Encode, Encoder, Encoder2,
- Encoder3, Encoder4, Encoder6, EncoderByteIter, ExactSizeEncoder, SliceEncoder,
+ ArrayEncoder, ArrayRefEncoder, BytesEncoder, check_encoder, Encode, Encoder, Encoder2, Encoder3,
+ Encoder4, Encoder6, EncoderByteIter, ExactSizeEncoder, SliceEncoder,
};
struct TestBytes<'a>(&'a [u8]);
diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs
index 2d9136cc..745293cb 100644
--- a/fuzz/src/lib.rs
+++ b/fuzz/src/lib.rs
@@ -48,7 +48,7 @@ where
loop {
let mut chunk = encoder.current_chunk();
while !chunk.is_empty() && decoder.push_bytes(&mut chunk).unwrap() {}
- if !chunk.is_empty() || !encoder.advance() {
+ if !chunk.is_empty() || encoder.advance().has_finished() {
break;
}
}
diff --git a/hashes/src/lib.rs b/hashes/src/lib.rs
index eccb7d73..7671ce27 100644
--- a/hashes/src/lib.rs
+++ b/hashes/src/lib.rs
@@ -212,7 +212,7 @@ where
{
loop {
engine.input(encoder.current_chunk());
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
diff --git a/io/src/lib.rs b/io/src/lib.rs
index cc9bfeb2..633416aa 100644
--- a/io/src/lib.rs
+++ b/io/src/lib.rs
@@ -481,7 +481,7 @@ where
{
loop {
writer.write_all(encoder.current_chunk())?;
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
diff --git a/p2p/src/address.rs b/p2p/src/address.rs
index 3fda3b40..d899a9ad 100644
--- a/p2p/src/address.rs
+++ b/p2p/src/address.rs
@@ -13,7 +13,7 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV
use arbitrary::{Arbitrary, Unstructured};
use encoding::{
ArrayDecoder, ArrayEncoder, ByteVecDecoder, BytesEncoder, CompactSizeEncoder,
- CompactSizeU64Decoder, Decoder2, Decoder4, Encoder2, Encoder4,
+ CompactSizeU64Decoder, Decoder2, Decoder4, EncoderStatus, Encoder2, Encoder4,
};
use internals::array::ArrayExt;
@@ -417,32 +417,32 @@ impl<'e> encoding::Encoder for AddrV2Encoder<'e> {
&[]
}
- fn advance(&mut self) -> bool {
- if self.network.is_some() && !self.network.advance() {
+ fn advance(&mut self) -> EncoderStatus {
+ if self.network.is_some() && self.network.advance().has_finished() {
self.network = None;
- return true;
+ return EncoderStatus::HasMore;
}
- if self.size.is_some() && !self.size.advance() {
+ if self.size.is_some() && self.size.advance().has_finished() {
self.size = None;
- return true;
+ return EncoderStatus::HasMore;
}
- if self.bytes4.is_some() && !self.bytes4.advance() {
+ if self.bytes4.is_some() && self.bytes4.advance().has_finished() {
self.bytes4 = None;
- return false;
+ return EncoderStatus::Finished;
}
- if self.bytes16.is_some() && !self.bytes16.advance() {
+ if self.bytes16.is_some() && self.bytes16.advance().has_finished() {
self.bytes16 = None;
- return false;
+ return EncoderStatus::Finished;
}
- if self.bytes32.is_some() && !self.bytes32.advance() {
+ if self.bytes32.is_some() && self.bytes32.advance().has_finished() {
self.bytes32 = None;
- return false;
+ return EncoderStatus::Finished;
}
- if self.nbytes.is_some() && !self.nbytes.advance() {
+ if self.nbytes.is_some() && self.nbytes.advance().has_finished() {
self.nbytes = None;
- return false;
+ return EncoderStatus::Finished;
}
- true
+ EncoderStatus::HasMore
}
}
@@ -1556,7 +1556,7 @@ mod test {
assert_eq!(encoder.len(), total_len - bytes_consumed);
bytes_consumed += chunk_len;
- if !encoder.advance() {
+ if encoder.advance().has_finished() {
break;
}
}
diff --git a/p2p/src/merkle_tree.rs b/p2p/src/merkle_tree.rs
index d9c61b84..628257cf 100644
--- a/p2p/src/merkle_tree.rs
+++ b/p2p/src/merkle_tree.rs
@@ -15,8 +15,8 @@ use alloc::vec::Vec;
#[cfg(feature = "arbitrary")]
use arbitrary::{Arbitrary, Unstructured};
use encoding::{
- ArrayDecoder, ArrayEncoder, ByteVecDecoder, CompactSizeEncoder, Decoder2, Decoder3, Encoder2,
- Encoder3, SliceEncoder, VecDecoder,
+ ArrayDecoder, ArrayEncoder, ByteVecDecoder, CompactSizeEncoder, Decoder2, Decoder3,
+ EncoderStatus, Encoder2, Encoder3, SliceEncoder, VecDecoder,
};
use internals::ToU64 as _;
use primitives::block::{self, Block, Checked, HeaderDecoder, HeaderEncoder};
@@ -472,9 +472,9 @@ impl encoding::Encoder for BitVecEncoder {
}
}
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
self.exhausted = true;
- false
+ EncoderStatus::Finished
}
}
diff --git a/p2p/src/message.rs b/p2p/src/message.rs
index 5cf3e25d..986c4915 100644
--- a/p2p/src/message.rs
+++ b/p2p/src/message.rs
@@ -14,8 +14,8 @@ use core::{fmt, mem};
#[cfg(feature = "arbitrary")]
use arbitrary::{Arbitrary, Unstructured};
use encoding::{
- self, ArrayDecoder, ArrayEncoder, BytesEncoder, CompactSizeEncoder, Decoder2, Encoder2,
- SliceEncoder, VecDecoder,
+ self, ArrayDecoder, ArrayEncoder, BytesEncoder, CompactSizeEncoder, Decoder2, EncoderStatus,
+ Encoder2, SliceEncoder, VecDecoder,
};
use hashes::{sha256d, HashEngine};
use primitives::block::{self, HeaderDecoder, HeaderEncoder};
@@ -150,7 +150,7 @@ impl encoding::Encoder for CommandStringEncoder {
fn current_chunk(&self) -> &[u8] { self.0.current_chunk() }
#[inline]
- fn advance(&mut self) -> bool { self.0.advance() }
+ fn advance(&mut self) -> EncoderStatus { self.0.advance() }
}
impl encoding::ExactSizeEncoder for CommandStringEncoder {
@@ -1036,7 +1036,7 @@ impl encoding::Encoder for NetworkMessageEncoder<'_> {
}
}
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
match self {
Self::Version(e) => e.advance(),
Self::Addr(e) => e.advance(),
@@ -1066,7 +1066,7 @@ impl encoding::Encoder for NetworkMessageEncoder<'_> {
Self::FeeFilter(e) => e.advance(),
Self::AddrV2(e) => e.advance(),
Self::SendTxRcnCl(e) => e.advance(),
- Self::Empty => false,
+ Self::Empty => EncoderStatus::Finished,
Self::Unknown(e) => e.advance(),
}
}
@@ -1548,7 +1548,7 @@ impl encoding::Encoder for V2NetworkMessageEncoder<'_> {
}
}
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
match self {
Self::ShortId(e) => e.advance(),
Self::FullCommand(e) => e.advance(),
diff --git a/primitives/src/transaction.rs b/primitives/src/transaction.rs
index 1db769f5..b4d1c9b0 100644
--- a/primitives/src/transaction.rs
+++ b/primitives/src/transaction.rs
@@ -16,7 +16,7 @@ use core::{cmp, mem};
#[cfg(feature = "arbitrary")]
use arbitrary::{Arbitrary, Unstructured};
-use encoding::{ArrayEncoder, BytesEncoder, Encoder2};
+use encoding::{ArrayEncoder, BytesEncoder, Encoder2, EncoderStatus};
#[cfg(feature = "alloc")]
use encoding::{
CompactSizeEncoder, Decoder2, Decoder3, Encode as _, Encoder3, Encoder6, SliceEncoder,
@@ -800,16 +800,16 @@ impl encoding::Encoder for WitnessesEncoder<'_> {
}
#[inline]
- fn advance(&mut self) -> bool {
+ fn advance(&mut self) -> EncoderStatus {
let Some(cur) = self.cur_enc.as_mut() else {
- return false;
+ return EncoderStatus::Finished;
};
loop {
// On subsequent calls, attempt to advance the current encoder and return
// success if this succeeds.
- if cur.advance() {
- return true;
+ if cur.advance().has_more() {
+ return EncoderStatus::HasMore;
}
// self.inputs guaranteed to be non-empty if cur_enc is non-None.
self.inputs = &self.inputs[1..];
@@ -818,11 +818,11 @@ impl encoding::Encoder for WitnessesEncoder<'_> {
if let Some(input) = self.inputs.first() {
*cur = input.witness.encoder();
if !cur.current_chunk().is_empty() {
- return true;
+ return EncoderStatus::HasMore;
}
} else {
self.cur_enc = None; // shortcut the next call to advance()
- return false;
+ return EncoderStatus::Finished;
}
}
}
diff --git a/primitives/src/witness.rs b/primitives/src/witness.rs
index a0dee273..ad349468 100644
--- a/primitives/src/witness.rs
+++ b/primitives/src/witness.rs
@@ -12,7 +12,7 @@ use arbitrary::{Arbitrary, Unstructured};
#[cfg(doc)]
use encoding::Decoder4;
use encoding::{
- self, BytesEncoder, CompactSizeDecoder, CompactSizeEncoder, Decoder as _, Encoder2,
+ self, BytesEncoder, CompactSizeDecoder, CompactSizeEncoder, Decoder as _, EncoderStatus, Encoder2,
};
#[cfg(feature = "hex")]
use hex::DecodeVariableLengthBytesError;
@@ -299,7 +299,7 @@ impl encoding::Encoder for WitnessEncoder<'_> {
fn current_chunk(&self) -> &[u8] { self.0.current_chunk() }
#[inline]
- fn advance(&mut self) -> bool { self.0.advance() }
+ fn advance(&mut self) -> EncoderStatus { self.0.advance() }
}
/// The decoder for the [`Witness`] type.
Risk score
Our methodology →Why this scored 19/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.