💬 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

👍 tdb3 approved a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/31586#pullrequestreview-2526910770)
ACK 2bdaf52ed1259fd3bec22b680e12563fcee0a8b3

Dry ran the guide with a fresh NetBSD installation, built with zmq, tested zmqpubhashblock with an adhoc python client (saw the topic, block hash, and sequence arrive when generating blocks in regtest with `-generate`).

Also successfully ran the functional test suite (python). Had to create a symlink (`python3` --> `python3.10`).

💬 yancyribbens commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2567108046)
> Right, but I think the question is whether negative fee rates should be rounded away from or towards zero

`ceil` rounds towards zero: `ceil(-2.4) = -2.000000`. Rounding toward zero when negative is correct imo. Rounding up is done to make sure the fee is adequate instead of falling short.

> Also, I'm not sure what kind of scenario a negative fee rate would be used.

Good question. frameworks like rust-bitcoin use an unsigned int for FeeRate so it can't be negative.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567112162)
@darosior @Sjors @pinheadmz

I've recently analyzed the blockchain for the last two years from 4th December 2022 to 23rd December 2024 to move this forward.

I will cross-write my findings here, but more seen here details can be seen here https://delvingbitcoin.org/t/analyzing-mining-pool-behavior-to-address-bitcoin-cores-double-coinbase-reservation-issue/1351

- Most mining pools adhere to Bitcoin Core’s default setup, keeping coinbase transaction weights well below **4000 WU**. However,
...

📝 theStack opened a pull request: "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds"
(https://github.com/bitcoin/bitcoin/pull/31588)
The determinstic line coverage checking script `test_deterministic_coverage.sh` was seemingly forgotten to be adapted in the course of switching from autotools to CMake (only the error messages containing build instructions were updated in a9964c04447745435747d9cc557165c43902783b / #30875). Since `gcovr` needs to access both the source and build files, the path to the latter has to be passed explicitly as they are not in the same anymore (see e.g. https://github.com/gcovr/gcovr/issues/340).

C
...

💬 tcharding commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2567132185)
If I'm not mistaken (again) this line of code can never be hit.

`if (nSatoshisPerK > 0) nFee = CAmount(1);`

💬 theStack commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2567137879)
Thanks for the reviews! I agree that skipping the test is a bad idea, switched to to an explicit error message instead as suggested, also added a log.info with instructions on how to obtain the binaries for previous releases in this case.

💬 sipa commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2567140752)
I believe the only way to hit negative feerates is when using the `prioritizetransaction` RPC with a (highly) negative fee_delta argument. I don't know why in this context rounding away from zero (as `CFeeRate` does) would matter.

Longer term, I think `CFeeRate` should be replaced with, or implemented using, `FeeFrac` which avoids divisions.

🤔 ariard reviewed a pull request: "p2p: Fill reconciliation sets (Erlay) attempt 2"
(https://github.com/bitcoin/bitcoin/pull/30116#pullrequestreview-2526935458)
Reviewed up to commit feb8c98db (“p2p: Add transactions to reconciliation sets…”).

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1900472039)
If the peer is `m_wtxid_relay=false` should `m_tx_inven tory_known_filter` be checked by `txid` ? Note, given the parent identifiers are retrieved from `GetMemPoolParents()` we can have the identifiers easily. This could avoid some duplicated INV flooding.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1900468228)
I think the rationale for introducing `SortPeersByFewestParents` could be more fleshed out, i.e iiuc explaining that eligible peers with the lowest number of parents for a target transaction in their reconciliation sets are favored for fan-out announcements, as this can be a hint of better connectivity in the overall transaction-relay graph and we wish to propagate the target transaction faster (..?).