💬 maflcko commented on pull request "refactor: Remove redundant edge case in fee rate rounding logic":
(https://github.com/bitcoin/bitcoin/pull/31572#issuecomment-2566313057)
> Addresses #31558. This fix improves code readability.

I don't think it does. Someone will need to investigate why the behavior is not symmetric for negative feerates and whether making it symmetric is a bugfix. See https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2560984394:

> I haven't checked if such a change is a bugfix or behavior change for prioritisetransaction, so it would be good to check that as well.

💬 maflcko commented on pull request "txmempool: fix typos in comments":
(https://github.com/bitcoin/bitcoin/pull/31584#issuecomment-2566317181)
lgtm ACK 34e8ee23b83eeecb1c4022b48e7b8db45a60ffdd

📝 hebasto opened a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/31586)
This PR:

1. Updates the documented NetBSD version.

2. Adds the optional ZeroMQ package to align the guide with other *BSD systems.

3. Updates the Python version to meet the minimum requirement specified in https://github.com/bitcoin/bitcoin/pull/30527.

4. Suggests to Install [`net/py-zmq`](https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/net/py-zmq/index.html) package to enable the `interface_zmq.py` functional test.

5. Fix a formatting issue.

See also the recent NetBSD nightly
...

👍 hebasto approved a pull request: "txmempool: fix typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31584#pullrequestreview-2526353028)
ACK 34e8ee23b83eeecb1c4022b48e7b8db45a60ffdd.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2566337776)
Added a helper function `NextEmptyBlockIndex`, which also fixes the missing nBits value on mainnet and signet.

💬 jaeheonshim commented on pull request "refactor: Remove redundant edge case in fee rate rounding logic":
(https://github.com/bitcoin/bitcoin/pull/31572#discussion_r1900111243)
Sorry, that `if` statement is unnecessary. I must have missed it.

✅ jaeheonshim closed a pull request: "refactor: Remove redundant edge case in fee rate rounding logic"
(https://github.com/bitcoin/bitcoin/pull/31572)

💬 jaeheonshim commented on pull request "refactor: Remove redundant edge case in fee rate rounding logic":
(https://github.com/bitcoin/bitcoin/pull/31572#issuecomment-2566452453)
What @maflcko said makes sense. I'll do some more investigating.

🤔 tdb3 reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2526474614)
Approach ACK

Planning to test later today as time allows. Left a minor comment, but it could wait until after testing (to avoid churn).

💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1900114122)
Let's add release notes for this, `getdifficulty`, and `gettarget`, e.g. something like:

```
Updated RPCs
---
- `getmininginfo` now returns the current target in the `target` field, and a `next` object, which specifies the `height`, `nBits`, `difficulty`, and `target` for the next block.
- `getdifficulty` can now return the difficulty for the next block (rather than the current tip) when calling with the boolean `next` argument set to true.

New RPCs
---
- `gettarget` can be used to r
...

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2566495339)
Added a testnet4 test, which includes the first 2016 real blocks. This takes up 1 MB (IIUC git effectively gzips that into 250kb).

Also added release notes.

I moved the `NextEmptyBlockIndex` helper from `node/miner.h` to `rpc/util.h`.

💬 jaeheonshim commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2566496077)
@maflcko I'd like to work on this issue, but this being my first time contributing to Bitcoin Core, I'm a bit lost. Where should I look to figure out exactly how the rounding for the fee mechanism should be implemented?

💬 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