💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2566509580)
> I moved the `NextEmptyBlockIndex` helper from `node/miner.h` to `rpc/util.h`.

That didn't work, because `rpc/util.cpp` lives in `libbitcoin_common`, while the `UpdateTime` function it calls lives in `libbitcoin_node`. I moved it to `rpc/server_util.{h,cpp}` instead.

🤔 l0rinc reviewed a pull request: "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC"
(https://github.com/bitcoin/bitcoin/pull/31179#pullrequestreview-2526455183)
Approach ACK
There are a LOT of other places where we will be able to use this - we should do them in follow-up PRs

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900120479)
we're repeating the same request 3 times, might as well extract it:
```suggestion
if (auto witness{tx.vin[i].scriptWitness}; !witness.IsNull()) {
UniValue txinwitness(UniValue::VARR);
txinwitness.reserve(witness.stack.size());
for (const auto& item : witness.stack) {
txinwitness.push_back(HexStr(item));
}
in.pushKV("txinwitness", std::move(txinwitness));
}
```

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900101802)
:+1: for extracting the common part

nit: in `blockToJSON` this is simply called `verbosity`

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900164051)
the real RPC also [reads the block from disk](https://github.com/bitcoin/bitcoin/blob/master/src/rest.cpp#L322), we could bench that as well - instead of assuming it's already in memory (this will also enable additional optimization opportunities)

💬 fjahr commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2566587452)
tACK faf7eac364fb7f421a649b483286ac8681d92b31

Reviewed code and verified that tests are still running as intended.

💬 fjahr commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#issuecomment-2566590949)
Code review ACK fa83bec78ef3e86445e522afa396c97b58eb1902

📝 Gudnessuche opened a pull request: "Enhance fee estimation logic and improve error handling in TxConfirmS…"
(https://github.com/bitcoin/bitcoin/pull/31587)
- Added reserve calls in TxConfirmStats constructor to avoid multiple reallocations for txCtAvg and m_feerate_avg vectors.
- Added a check in resizeInMemoryCounters to ensure newbuckets is greater than 0, throwing an invalid_argument exception if not.
- Improved error handling in Record method by adding a check to throw an out_of_range exception if blocksToConfirm exceeds the maximum tracked periods.
- Fixed the missing parenthesis and completed the logic in the Read method to ensure proper s
...

👍 l0rinc approved a pull request: "refactor: Allow std::byte in Read(LE/BE)"
(https://github.com/bitcoin/bitcoin/pull/31524#pullrequestreview-2526656628)
ACK fa83bec78ef3e86445e522afa396c97b58eb1902

Please see if any of the simplification suggestions apply, but I'm fine with it as is

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900234707)
nit: given the simplicity of this PR, if you edit again, consider if it would be simpler to use `sizeof` expressions instead of the constants:
```suggestion
memcpy(&x, ptr, sizeof(uint16_t));
```

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900236190)
We could const the parameter and locals to obviate which one we're writing:
```suggestion
template <ByteType B>
void WriteLE16(B* ptr, const uint16_t x)
{
const uint16_t v{htole16_internal(x)};
memcpy(ptr, &v, 2);
}
```

💬 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`.