💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729323391)
Interesting idea!

Should move it out of the `detail` namespace. That makes the naming of `Hex` more important. Are people still happy with it?

🤔 tdb3 reviewed a pull request: "contrib: add dockerfile for building image fron source code"
(https://github.com/bitcoin/bitcoin/pull/30702#pullrequestreview-2257777209)
Thank you for your interest in the project. Based on the following, I'm going to have to Concept NACK this one.

> ~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.

I agree with this. I think it's great if developers/users would like to dockerize builds independently, but this doesn't seem like something that should be added to this repository. Addi
...

💬 tdb3 commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#discussion_r1729318386)
Storing a duplicate bitcoin.conf (when one can be generated with `get-bitcoin-conf.sh` directly from `bitcoind --help`) would lead to increased maintenance effort.

💬 l0rinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2307590054)
After yesterday's OS update (might be unrelated, not sure), I started getting:
```bash
% cmake -B build && cmake --build build -j8 && build/src/test/test_bitcoin --run_test=coins_tests
...

[ 70%] Linking CXX executable test_bitcoin
ld: warning: ignoring duplicate libraries: '../secp256k1/src/libsecp256k1.a'
duplicate symbol '__ZN16sigopcount_tests13GetSigOpCount11test_methodEv' in:
build/src/test/CMakeFiles/test_bitcoin.dir/sigopcount_tests.cpp.o
build/src/test/CMakeFiles/test_
...

💬 brunoerg commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2307612607)
Approach NACK

Unnecessary refactoring and tbh I could not clear understand the motivation and improvement.

👍 brunoerg approved a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703#pullrequestreview-2257865213)
utACK fa5aeab3cb18405ecf8a1401d89539b924a618f6

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2307616617)
> not clear understand the motivation and improvement.

The motivation was to add every block's reward when calculating total, not just every 1000th (multiplied)

💬 theuni commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2307711611)
@hebasto Didn't we decide in last week's IRC meeting to delete configure.ac + Makefile.* in this PR, then doing a full removal (#30664) as a follow-up? That way the conflicts are apparent as soon as it's merged?

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2307729771)
> @hebasto Didn't we decide in last week's IRC meeting to delete configure.ac + Makefile.* in this PR, then doing a full removal (#30664) as a follow-up? That way the conflicts are apparent as soon as it's merged?

I understood it as an intention to reveal conflicts. A follow up that just removes the code should be a no-brainer, right?

👍 tdb3 approved a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703#pullrequestreview-2258012036)
tested ACK fa5aeab3cb18405ecf8a1401d89539b924a618f6
Seems to work well and is an elegant simplification. Thank you.

Not that it seems to matter for callers of `get_previous_releases.py`, but just noting that previously `download_binary()` returned 1 on 404, while the update returns 22 (as documented in curl's man page, ...at least with `curl 7.88.1 (x86_64-pc-linux-gnu)`).


previous:
```
$ test/get_previous_releases.py -b v29.0
Releases directory: releases
Fetching: https://bitcoinc
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729491937)
@stickies-v:
> It would, in my view, make it more confusing though, by hiding that these operators are in fact consteval. I might be missing some nuance here, though?

Sorry for not addressing this before, was a bit rushed in my previous post.
My understanding is the `Hex`-instantiations are `consteval`, and the operators are *instantiated* at compile time, but the last two are commonly *executed* at runtime (with a compile time baked `Hex`-value).

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729499254)
Will see if all compilers are fine with removing `util::detail::Hex(...)` here. Hopefully they are!

I prefer documenting the full type in for these specific tests, but admit it is quite verbose.

✅ Christewart closed a pull request: "Implement 64 bit arithmetic op codes in the Script interpreter"
(https://github.com/bitcoin/bitcoin/pull/29221)

💬 Christewart commented on pull request "Implement 64 bit arithmetic op codes in the Script interpreter":
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-2307855599)
Too speculative for now https://delvingbitcoin.org/t/64-bit-arithmetic-soft-fork/397/53

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563315)
Hm, doesn't this code catch this already?

```
FILE* file{fsbridge::fopen(temppath, "wb")};
AutoFile afile{file};
if (afile.IsNull()) {
```

I get this here:

```
$ src/bitcoin-cli dumptxoutset \~/Downloads/utxo.dat latest
error code: -8
error message:
Couldn't open file /Users/XXX/Library/Application Support/Bitcoin/~/Downloads/utxo.dat.incomplete for writing.
```

Can you give a more detailed example to reproduce this maybe?

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563346)
done

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563423)
Leaving this for a follow-up.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563485)
I agree that `-height=XXXX` is better but having `-rollback` and `-height=XXXXX` seems worse than `rollback` and `-rollback=XXXXX` and of course just `-height` doesn't make sense. So leaving this as is for now.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563501)
done

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563563)
added