💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682777414)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682782077)
Still kept it as an overload as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d. If you prefer different names for runtime/compile time, which do you suggest?

💬 hebasto commented on pull request "depends: Amend handling flags environment variables":
(https://github.com/bitcoin/bitcoin/pull/30477#issuecomment-2236426928)
> It would be good to show an example of what this actually fixes.

It is required only for the main CMake-based build system.

For instance, CMake [assumes](https://cmake.org/cmake/help/latest/envvar/CXXFLAGS.html) that CXXFLAGS affects the content of the CMAKE_CXX_FLAGS variable. However, without this change, the CMAKE_CXX_FLAGS_RELEASE or CMAKE_CXX_FLAGS_DEBUG will be affected instead.

The PR description has been updated.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2236512167)
@ismaelsadeeq thanks, updated the description.

💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2236517862)
(Fixed QT/GUI CI errors).

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682825302)
Why add a consteval overload here? Generally I am not a fan of adding test-only code (code that is only used in tests) to the real program. Performance or exe-size shouldn't matter for tests, unless it is significant. If you still want to add it to the tests, that is fine, but then please move it to the test code. But please change the function name of the test-only function to something else. It seems odd that test-only code forces real code to be renamed.

💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682842021)
Happy to push a change that changes this, if someone writes one. Also, I am happy to review a pull request doing the change, if someone opens one.

But this requires changing more lines of code, so I didn't find it worthwhile to do by myself.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682842951)
No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to `BlockCreateOptions`. It seems worthy of a positional argument.

💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682843219)
Good call, will fix.

💬 fanquake commented on pull request "guix: bump time-machine to e6e70abe65850da0ae69428654375d367088ef5e":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2236543931)
Bumped this further as https://git.savannah.gnu.org/cgit/guix.git/commit/?id=49fa48eed78c50d9674e9868e5a2b3e029a6e403 landed.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2236563617)
Guix Build (aarch64):
```bash
e9a70ec7c66652aa96facc2fe3117fb177cb61e6d28fadc48893546f9cebf6cb guix-build-218e69c705d7/output/aarch64-linux-gnu/SHA256SUMS.part
0fea765c772ee00ec37d4efde6296e173a8a765792534a731e64b253effd9242 guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu-debug.tar.gz
1689943e2b3caa664a9bb5c137ffc161252b253568b563d5a4433e2ac55174ec guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu.tar.gz
6934a4
...

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682878016)
A more flexible alternative would be to just accept a string_view and rely on the length assert. However, I guess this prints a more confusing compile error.

Seems fine to change later to string_view, if this is needed.

However, I wonder if you can replace `65` by `WIDTH+1`?

💬 paplorinc commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682881489)
I means simply:
```patch
===================================================================
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
--- a/src/test/fuzz/txorphan.cpp (revision 37992244e636e52e0c2baff1bc5f36e60d9c956f)
+++ b/src/test/fuzz/txorphan.cpp (date 1721306232235)
@@ -37,11 +37,11 @@
SetMockTime(ConsumeTime(fuzzed_data_provider));

TxOrphanage orphanage;
- std::set<COutPoint> outpoints;
+ std::vector<COutPoint> outpoints;

/
...

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682882026)
Also, I wonder if the three duplicate redirects can be replaced by a single `uinsg base_blob::base_blob;` (or similar) to import all constructors.

💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682889408)
Yeah, I guess, given that duplicates are tolerated either way

👍 theStack approved a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453#pullrequestreview-2185928833)
tACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15

Thanks for following up! Tested this with the following patch, where the introduced debug messages would pop up (with received message type "version" as expected) in the log every few runs:

<details>
<summary>Patch</summary>

```diff
diff --git a/src/net.h b/src/net.h
index a4e4b50360..8b9b0003b8 100644
--- a/src/net.h
+++ b/src/net.h
@@ -747,6 +747,13 @@ public:
std::optional<std::pair<CNetMessage, bool>> PollMessage()

...

💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236633461)
> Electrum have adopted a practical approach, but it's not entirely correct. They consider all conflicting transactions, and then discard according to the hierarchy confirmed > mempool > local ([reference](https://github.com/spesmilo/electrum/blob/76f5d6ceb07b49e403f63488832fd6cb397ee91b/electrum/address_synchronizer.py#L295-L298)). This does not consider policy rules, and may result in discarding a transaction that is later mined.

Yes, but that should be harmless, as a confirmed transaction
...

💬 glozow commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2236638869)
concept ACK

💬 maflcko commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2236639377)
> I think thematically the test would also fit very well into `p2p_handshake.py`, but no strong feelings.

Sure, I may move-only it, if I have to re-touch for other reasons.

⚠️ luisschwab opened an issue: "`scantxoutset` should output the block hash instead of block height"
(https://github.com/bitcoin/bitcoin/issues/30478)
### Please describe the feature you'd like to see added.

When using the `scantxoutset` RPC, the current behaviour is to show the block **height** of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query. In this case, an UTXO that does not exist would be assumed to exist, unless the chain's tip hash is recorded before the scan, and make sure it still exists after, as per [this](https://github.com/
...