💬 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 (..?).

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

If the goal is to prevent a fee of zero, it probably should be: `if (nSatoshisPerK == 0) nFee = CAmount(1);`

💬 sipa commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2567180312)
For context, support for negative feerates in `CFeeRate` was added in #7796.

✅ maflcko closed a pull request: "Enhance fee estimation logic and improve error handling in TxConfirmS…"
(https://github.com/bitcoin/bitcoin/pull/31587)

💬 tdb3 commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2567204709)
Looks like we can re-use the new `DIFF_1_N_BITS` and `REGTEST_N_BITS` in a few other tests (https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/). Could be added if there are other changes or left for future follow-up. The use of `REGTEST_N_BITS` might be borderline (the test is creating blocks by hand).

⚠️ Pttn opened an issue: "P2TR Spending Bug - Signing Transaction Failed"
(https://github.com/bitcoin/bitcoin/issues/31589)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Some Segwit Version 1 Addresses generated with [Achow101's Rust-Vanitygen](https://github.com/achow101/rust-vanitygen) cause an issue when spending coins using them. Generally, the "Signing transaction failed" error occurs and the transaction is not created. Though, a workaround still allows to spend the coins.

I don't know if the issue is exclusive to P2TR, but never encountered such p
...

💬 achow101 commented on issue "P2TR Spending Bug - Signing Transaction Failed":
(https://github.com/bitcoin/bitcoin/issues/31589#issuecomment-2567270082)
It looks like this is due to the pubkey evenness bit. The internal key is `03`, and this information is known and in memory after importing. But after reloading the wallet, the evenness bit is lost and since the public descriptor stored in the wallet only has the xonly pubkey, so it defaults to `02`. Then the signing code is looking for the privkey to the `02` pubkey rather than the `03`, is unable to find it, and therefore fails to sign.

This should only be a problem for tr() descriptors wit
...

💬 jimbrend commented on issue "P2TR Spending Bug - Signing Transaction Failed":
(https://github.com/bitcoin/bitcoin/issues/31589#issuecomment-2567287843)
No.

📝 achow101 opened a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590)
When a `ConstPubkeyProvider` is xonly, the stored pubkey does not necessarily have the correct evenness. `ToPrivateString()` is correctly handling this by looking up the keys for both evenness bits, but `GetPrivKey` does not. This results in not finding the private key when it is actually available if its pubkey is the other evenness.

To fix this, this key finding is refactored into `GetPrivKey()` so that its behavior is corrected, and `ToPrivateString()` is changed to use `GetPrivKey()` as w
...

💬 jonatack commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#issuecomment-2567294020)
Concept ACK

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567472713)
@ismaelsadeeq thanks for the research!

Ocean in the DATUM documentation indeed requires a reduced `-blockmaxweight=3985000`, so presumably they do something similar in their regular stratum pool: https://github.com/OCEAN-xyz/datum_gateway?tab=readme-ov-file#node-configuration

I still think we should use `-blockmaxweight` (default `4000000`) to refer to the entire block, including coinbase, and then have a separate `-maxcoinbaseweight` (default `8000`). This keeps the same behavior for mine
...

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2567475848)
@tdb3 thanks, I'll cherry-pick those two commits if any other changes are needed here.

💬 hebasto commented on pull request "doc: Update NetBSD Build Guide":
(https://github.com/bitcoin/bitcoin/pull/31586#issuecomment-2567496750)
@tdb3

Thank you for reviewing!

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

A symlink won't be necessary after https://github.com/bitcoin/bitcoin/pull/31541.

💬 hebasto 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-2567511345)
@kelvinator07

Could you provide please the `CMakeFiles/CMakeConfigureLog.yaml` file from your build directory?

👍 TheCharlatan approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2527313983)
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31