💬 polespinasa commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2019840095)
> Wouldn't it be better to set this to `static_cast<uint32_t>(nHeight +99 );`?
That would make the transaction invalid and unable to be included in a block until the `nHeight + 99` block is not a condition on when it can be spent, but when it is valid (therefore it can be mined).

💬 arejula27 commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2019840538)
True :(

💬 arejula27 commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2019842504)
That's true, I confused it with CLTV.

💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-2763808758)
Renamed the `flags` parameter to `mode`, matching the analogous change in the BIP draft.
Rebased to the latest master.

Still trying to locally reproduce the unit test failures.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019899624)
Sorry, that the builder *doesn't* outlive the graph. I was testing to see what happens when it *does* outlive it, and it's not crashing currently.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019900371)
feel free to resolve

💬 mabu44 commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019943385)
nit: sub**s**titute

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019968139)
Thanks! Fixed.

💬 Prabhat1308 commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2764219862)
re-ACK [`56f271e`](https://github.com/bitcoin/bitcoin/pull/32134/commits/56f271e9b9c82f40054d63d4b638584bd2faef00)

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019968258)
commit message still contains `seen_multipath`

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019968705)
this implicit empty constructor was always weird to me, a more explicit and uniform alternative could be:
```suggestion
substitutes = {};
```

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019970351)
To me `.emplace()` appears more specific. Assigning `{}` seems to make it `std::nullopt`, leading to test failures.

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019969499)
It also contains:

> Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019971150)
ok, if that's deliberate, please resolve this

💬 l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2019971282)
My mistake, thanks for checking

👍 l0rinc approved a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2727715824)
utACK 56f271e9b9c82f40054d63d4b638584bd2faef00

🤔 Prabhat1308 reviewed a pull request: "fuzz: Make partially_downloaded_block more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32158#pullrequestreview-2727721222)
tACK [`fa1e299`](https://github.com/bitcoin/bitcoin/pull/32158/commits/fa1e2995d9996641e79f92549e99a6b37e36d140)

Tested 10 runs with each 32 and 128 parallel threads on MacOS.
Steps followed
```
cmake -B build -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DBUILD_FOR_FUZZING=ON
...

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2764238498)
Also seeing warnings regarding locale (https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2762968613, https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2763240357). (Which one would hope for reproducible builds).

Guix (x86_64 NixOS):
```
6ea4a76be3383337e57d6a12450bd589776ebb3fd0d9161347766ef845241e13 guix-build-c4861570e468/output/aarch64-linux-gnu/SHA256SUMS.part
3eb7656483dfe47fa6b7cf40bceb3decda73474c813edb224d42840adb8b49d6 guix-build-c4861570e468/output/aarch64-lin
...

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2764240416)
i built with the following patch to set a UTF-8 locale check if there would be a difference:
```patch
diff --git a/contrib/guix/libexec/build.sh b/contrib/guix/libexec/build.sh
index 26180bae24..47366c7e11 100755
--- a/contrib/guix/libexec/build.sh
+++ b/contrib/guix/libexec/build.sh
@@ -2,7 +2,8 @@
# Copyright (c) 2019-2022 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.p
...

👍 TheCharlatan approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2727393482)
crACK 7b6fe5a2196d53a3024dfe7f11b316a50acc3cb7

This reduces complexity overall and I am happy that the slightly verbose block in `AcceptBlockHeader` and the `m_dirty_blockindex` are gone. It is not really straight forward to review though and I hope I did not miss any edge cases or real performance drawbacks. Might be good to get some more eyes on this. Having a better abstraction on the BlockMap / block tree data structure that just handles all of this internally could make things easier in
...

💬 TheCharlatan commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2019820910)
In commit fbe9e2a391d5f6e14998776b03d4ffbbf43f33c8:

Nit: Should this change really be in this commit? It does make sense in the next commit. I don't think it would be a bug per se to never erase, but it might be less efficient.