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

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