💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900235591)
Byte type means that size is 1, right? If you edit again, consider simplifying to something like:
```suggestion
template <typename B>
concept ByteType = (sizeof(B) == 1);
```

Or if you don't like it, to the mentioned `uint8_t`
```suggestion
template <typename B>
concept ByteType = std::same_as<B, uint8_t> || std::same_as<B, std::byte>;
```

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900238844)
maybe we can roll back the loop now to something like:
```C++
void ChaCha20Aligned::SetKey(Span<const std::byte> key) noexcept
{
assert(key.size() == KEYLEN);
constexpr int words{KEYLEN / sizeof(uint32_t)};
for (int i{0}; i < words; ++i) input[i] = ReadLE32(key.data() + i * sizeof(uint32_t));
std::memset(&input[words], 0, (std::size(input) - words) * sizeof(uint32_t));
}
```

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900235081)
as far as I can tell inline templates are redundant here (`constexpr` also compiles, but not sure it's valid):
```suggestion
template <ByteType B>
uint16_t ReadLE16(const B* ptr)
```

💬 yancyribbens commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2566743551)
Maybe this issue should be closed and a new one started. The original issue states the code section isn't used, but it pretty clearly is, especially since there's even a test that shows it is. It is a bit of a mystery what the authors intent was and a code comment might have been nice to explain why if the `ceil` function resulted in zero and the size is not zero, then return -1 if fee_rate is negative or 1 if fee_rate is positive.

💬 adyshimony commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2566754776)
Try this to solve the issue:

1. brew install llvm
2. Clean build / delete dir to clearn cache
3. cmake -DCMAKE_C_COMPILER=/usr/local/opt/llvm/bin/clang -DCMAKE_CXX_COMPILER=/usr/local/opt/llvm/bin/clang++ -B build

💬 yancyribbens commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2566792854)
Actually, I think the "stale code" section is to prevent a fee of zero, and if the fee would be zero, then make it either 1 sat or -1 sat depending of if the fee_rate is positive or negative.

💬 jaeheonshim commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2566794961)
Right, but I think the question is whether negative fee rates should be rounded away from or towards zero. Also, I'm not sure what kind of scenario a negative fee rate would be used.

🤔 tdb3 reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2526714458)
Seems like there is some history here regarding regtest and fPowNoRetargeting=true (https://github.com/bitcoin/bitcoin/pull/6853). When configured to avoid internet connectivity, it seems reasonable to use testnet4 and canned data (`data/testnet4.hex`).

The bitcoin.conf for the test node has dnsseed=0, fixedseeds=0, and `connect=0`, which should help avoid internet traffic. Added a sleep in `run_test()` and didn't see internet traffic from the test node in Wireshark.

Planning to circle bac
...

💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900310948)
In the future, when testnet4 replaces testnet3 as `-testnet` (no trailing number), we'll need to drop the `4`.

💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900310982)
Similarly here (file rename in the future)

💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900275137)
Thinking out loud. nitty nit (feel free to ignore): Might not be obvious to the reader (the next block difficulty is the same as the current because there is no adjustment). If this file ends up being changed, could add a log message (or a comment), that is explicit.

e.g.
```python
self.log.info("Next difficulty should be the same as the current (no difficulty adjustment)")
assert_equal(self.nodes[0].getdifficulty(next=True), difficulty)
```

✅ tcharding closed an issue: "Stale code (that has no effect)"
(https://github.com/bitcoin/bitcoin/issues/31558)

💬 tcharding commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2566816739)
> In commit: https://github.com/bitcoin/bitcoin/commit/0fbaef9676a1dcb84bcf95afd8d994831ab327b6 a call to std::ceil was added which makes the following if statement do nothing.

Oh it seems I can't do integer division. `nFee` will be zero any time `0 < (nSatoshisPerK * nSize) < 1000`

I have no opinion on the rounding issue and will leave that to others to raise an issue if needed. Excuse the noise.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900345228)
Maybe, depends on how soon testnet5 shows up :-)

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900345259)
Will do if I need to retouch.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900345308)
Note that this pattern is copied from a similar testnet3 test.

💬 philmb3487 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1900345499)
just one nit, the OS is named 'macOS'

💬 zaidmstrr commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r1894636030)
Hey, I'm reviewing this PR; can you please explain to me the thoughts behind removing this line of code? 

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2526895295)
ACK 7f34802fd726b259f7df6f605e1d488faf251127


<details><summary>Tested on mainnet for 876960 difficulty adjustment</summary>

Expected: same diff/target in next block
```
$ build/src/bitcoin-cli getblockhash 876959
000000000000000000002428ab473fa8570115d4a000e637814b86248d35d370
$ build/src/bitcoin-cli invalidateblock 000000000000000000002428ab473fa8570115d4a000e637814b86248d35d370
$ build/src/bitcoin-cli getdifficulty
108522647629298.2
$ build/src/bitcoin-cli getdifficulty true
10
...

💬 maflcko commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2567097930)
What is your version of XCode? Ref: https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#1-xcode-command-line-tools