Skip to content

Added static_assert to check that base_blob is using whole bytes.#27929

Merged
fanquake merged 1 commit intobitcoin:masterfrom
Brotcrunsher:base_blob_whole_byte
Jun 27, 2023
Merged

Added static_assert to check that base_blob is using whole bytes.#27929
fanquake merged 1 commit intobitcoin:masterfrom
Brotcrunsher:base_blob_whole_byte

Conversation

@Brotcrunsher
Copy link
Copy Markdown
Contributor

Prior to this commit it was possible to create base_blobs with any arbitrary amount of bits, like base_blob<9>. One could assume that this would be a valid way to create a bit field that guarantees to have at least 9 bits. However, in such a case, base_blob would not behave as expected because the WIDTH is rounded down to the closest whole byte (simple integer division by 8). This commit makes sure that this oddity is detected and blocked by the compiler.

Prior to this commit it was possible to create base_blobs with any arbitrary amount of bits, like base_blob<9>. One could assume that this would be a valid way to create a bit field that guarantees to have at least 9 bits. However, in such a case, base_blob would not behave as expected because the WIDTH is rounded down to the closest whole byte (simple integer division by 8). This commit makes sure that this oddity is detected and blocked by the compiler.
@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Jun 21, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, theStack, stickies-v
Concept ACK Ayush170-Future

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27551 (Reduce calls to various SetNull methods by Bushstar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Jun 22, 2023

lgtm ACK 5fc4939

Copy link
Copy Markdown
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5fc4939

I doubt that we would ever instantiate base_blob with a template parameter that is not a multiple of 8, but of course it doesn't hurt to check this.

Tested with

diff --git a/src/uint256.cpp b/src/uint256.cpp
index 7f81c3c448..802f6d5642 100644
--- a/src/uint256.cpp
+++ b/src/uint256.cpp
@@ -63,6 +63,9 @@ template std::string base_blob<160>::ToString() const;
 template void base_blob<160>::SetHex(const char*);
 template void base_blob<160>::SetHex(const std::string&);

+// Test #27929
+template std::string base_blob<9>::GetHex() const;
+
 // Explicit instantiations for base_blob<256>
 template std::string base_blob<256>::GetHex() const;
 template std::string base_blob<256>::ToString() const;

leading to the failed static_assert as expected:

In file included from uint256.cpp:6:
./uint256.h:25:5: error: static_assert failed due to requirement '9U % 8 == 0' "base_blob currently only supports whole bytes."
    static_assert(BITS % 8 == 0, "base_blob currently only supports whole bytes.");
    ^             ~~~~~~~~~~~~~
uint256.cpp:67:22: note: in instantiation of template class 'base_blob<9>' requested here
template std::string base_blob<9>::GetHex() const;
                     ^
1 error generated.

Copy link
Copy Markdown
Contributor

@Ayush170-Future Ayush170-Future left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find! LGTM ACK.

Copy link
Copy Markdown
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5fc4939

@fanquake fanquake merged commit b741a62 into bitcoin:master Jun 27, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 27, 2023
…sing whole bytes.

5fc4939 Added static_assert to check that base_blob is using whole bytes. (Brotcrunsher)

Pull request description:

  Prior to this commit it was possible to create base_blobs with any arbitrary amount of bits, like base_blob<9>. One could assume that this would be a valid way to create a bit field that guarantees to have at least 9 bits. However, in such a case, base_blob would not behave as expected because the WIDTH is rounded down to the closest whole byte (simple integer division by 8). This commit makes sure that this oddity is detected and blocked by the compiler.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 5fc4939
  theStack:
    ACK 5fc4939
  stickies-v:
    ACK 5fc4939

Tree-SHA512: 6a06760f09d4a9e6f0b9338d4dddd4091f2ac59a843a443d9302959936d72c55f7cccd55a51ec3a5a799921f68be1b87968ef3c9c11d3389cbd369b5045bb50a
@bitcoin bitcoin locked and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants