Skip to content

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Mar 4, 2025

Follow-up to Add mainnet assumeutxo param at height 880,000, looking to simplify specifying explicit hashes in the source code.
It's not a full implementation yet, I'm looking for concept ack/nack and full CI feedback for now - applying it to the whole codebase may follow based on that.


This is also a follow-up to refactor: Replace ParseHex with consteval ""_hex literals, meant to simplify the usages (values beginning with the hexadecimal data, followed by the type conversion, similarly to sizeof(uint256) vs uint256::size()) and unifying hex parsing code.
Similarly to ""_hex, the ""_uint256 has the same compile-time constraints as its constructor (properly sized with actual hexadecimal values), but uses detail::Hex parser (reversed) (used in most other places for hex parsing) instead of base_blob(std::string_view hex_str).
The new implementation also makes it obvious that the values are reversed in this case.

I thought of removing the consteval_ctor hack as well, now that consteval conversion is claimed to work in visual studio, but since https://godbolt.org/z/j837req5h still seems to fail for latest x64 msvc v19, I postponed doing that.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 4, 2025

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31991.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept NACK maflcko

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:

  • #31974 (Drop testnet3 by Sjors)
  • #31910 (qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data by darosior)
  • #31649 (consensus: Remove checkpoints (take 2) by marcofleon)
  • #26201 (Remove Taproot activation height by Sjors)

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.

@l0rinc l0rinc force-pushed the l0rinc/implicit_hex_to_uint256 branch from 6a07360 to b340e03 Compare March 5, 2025 15:56
@l0rinc l0rinc changed the title RFC: Add implicit constructor for _hex to uint256 and remove consteval_ctor RFC: Add operator""_uint256 compile-time user-defined literal Mar 5, 2025
@l0rinc l0rinc force-pushed the l0rinc/implicit_hex_to_uint256 branch from b340e03 to fbbdbaa Compare March 9, 2025 16:24
@l0rinc l0rinc force-pushed the l0rinc/implicit_hex_to_uint256 branch from fbbdbaa to 1d06571 Compare March 11, 2025 15:58
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
@l0rinc l0rinc force-pushed the l0rinc/implicit_hex_to_uint256 branch from 1d06571 to 8c27aaf Compare March 14, 2025 13:51
@maflcko
Copy link
Member

maflcko commented Mar 14, 2025

I thought of removing the consteval_ctor hack as well, now that consteval conversion is claimed to work in visual studio, but since https://godbolt.org/z/j837req5h still seems to fail for latest x64 msvc v19, I postponed doing that.

I think the upstream bug link may be wrong. https://developercommunity.visualstudio.com/t/Can-not-initialize-array-with-consteval/10828473 looks closer

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

tend toward NACK for now, as I don't see the goal or benefit

Would be happy to ack a doc fix of the upstream msvc bug report

@@ -88,17 +88,17 @@ class CMainParams : public CChainParams {
consensus.signet_challenge.clear();
consensus.nSubsidyHalvingInterval = 210000;
consensus.script_flag_exceptions.emplace( // BIP16 exception
uint256{"00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"}, SCRIPT_VERIFY_NONE);
"00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"_uint256, SCRIPT_VERIFY_NONE);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the goal here is, or the improvement. Looks like extra code is added to do the same thing in two different ways?

@l0rinc l0rinc closed this Mar 14, 2025
@hodlinator
Copy link
Contributor

Would be happy to ack a doc fix of the upstream msvc bug report

Yes, it seems like our issue was closer this "sub-issue" in the currently linked issue: https://developercommunity.visualstudio.com/t/consteval-conversion-function-fails/1579014#T-N10550785 and they apparently only fixed the primary issue. :\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants