💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729201357)
just to be sure, was it intentional to use the bit-wise AND operator here, rather than the logical one (`&&=`), to avoid potential short-circuiting (see e.g. https://stackoverflow.com/questions/23107162/do-the-and-operators-for-bool-short-circuit)? IIUC, `add_extra_compact_tx` is always true at this point anyway (otherwise the if condition at the very top couldn't be true), so could also just do a regular assignment, I guess. But not sure what's better, one could argue that the current way is mo
...

💬 sancoder commented on issue "-proxy does not work for addresses like 10.x.y.z":
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2307425897)
> Bitcoin nodes keep a list of possible addresses of other nodes to connect to. This list is maintained by gossiping known addresses between nodes, including advertising own address to the other nodes, so that others know our address and can connect to us. From this perspective, it is useless to advertise an address like `10.x.y.z` to others on the internet because they will not be able to connect back. This is why we have a special `NET_UNROUTABLE` in the code.
Thank you for explaining this.

...

🤔 theStack reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2257664356)
lgtm, once again some smaller suggestions

💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729249780)
nit: using the pre-generated chain is the default, so could just remove this line

💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729251710)
I assume this line can also be removed

💬 theStack commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1729250880)
could use the `vout` result from `send_to_address` also here, so the `find_vout_for_address` helper isn't needed anymore

💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287555)
Yup, this is more coherent. Updated.

💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1729287638)
Updated

💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307515830)
@theStack thank you for the suggestions! just forcePushed a new commit with the changes.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729318481)
Since the `consteval`/`constexpr`/`inline` choices are not obvious, but have fairly solid reasoning behind them, I could add non-Doxygen comments like
```C++
// Calls non-constexpr function so can only be inline
inline auto operator""_hex_v_u8() { return std::vector<uint8_t>{UCharCast(...
``
What do you think?

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729321677)
That wouldn't explain to me why the compiler seems to allow it

💬 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?