💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2082066284)
Taken, thanks

💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2867201958)
Forced pushed to address recent comments see [c8a3fabe..31e3808d](https://github.com/bitcoin/bitcoin/compare/c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6..31e3808df9c59d36a07cad07df89ae1bf9e63000)

💬 laanwj commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2082115539)
Hahaha agree it would be extremely far-fetched to put data in a specific block, just to add it in the repository two years later.

📝 ismaelsadeeq opened a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463)
Attempt to fix #32461

In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.

https://github.com/bitcoin/bitcoin/blob/9a05b45da60d214cb1e5a50c3d2293b1defc9bb0/src/rpc/fees.cpp#L85

The test `test_feerate_mempoolminfee`, originally introduced in https://github.com/bitcoin/bitcoin/commit/ea31caf6b4c182c6f10f136548f84e603800511c, is incorrect.
It should append the higher value used to start the node
...

💬 ismaelsadeeq commented on issue "test: `acceptstalefeeestimates` failure in `feature_fee_estimation` after duplicate coinbase tx weight reservation fix":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2867564112)
@l0rinc Thanks for the thorough report.

However, the failure is not in the `acceptstalefeeestimates` subtest, but in `test_feerate_mempoolminfee`.

I believe the issue is that we are not appending `high_val` to `self.fees_per_kb` before checking the estimates.

I attempted to fix this in #32463.



---
Also, it's quite strange that the test started failing on this commit https://github.com/bitcoin/bitcoin/commit/6b165f5906fc53bd10bedff85a6ef26e0aabdc5c

I haven't looked into that yet.

💬 achow101 commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2867645207)
Fixed the linter and reorganized as suggested.

💬 l0rinc commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2867652250)
Thanks for fixing it so quickly, I can confirm that this fixes the test for the failing `--random=3450808900320758527`.
Someone more knowledgeable in this area of the code should also look at at, but from my part it's a lightweight ACK.

ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8

👍 TheCharlatan approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2829494150)
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a

Guix build (aarch64):
```
2d1c74e19e10c55a45e137f151f77f0e0628c96f0664949ba730c249e8065597 guix-build-ab878a7e7410/output/aarch64-linux-gnu/SHA256SUMS.part
26cb4ed3dcaf1bfc2d2f9cdc34e8648df11e2aed33bdeb8618a6d8a3370ba0e3 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-ab878a7e7410-aarch64-linux-gnu-debug.tar.gz
756e79f7a151b67edc7b607f4b3b0baa72025dbf7cc158e27485f885fbd6adc1 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-a
...

💬 JeremyRubin commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2867708417)
squashed the CI fixmes to get "test each commit" to pass.

@Sjors no one understands the annex, so you're not alone. You can see https://groups.google.com/g/bitcoindev/c/Q5j2Kb6XeHI?pli=1 for some context on efforts to "standardize" the annex currently ongoing.

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082406101)
I'm a bit hesitant, because this part of the code has more docs than usual, e.g.
```C++
/**
* Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
* as 20 sigops. With pay-to-script-hash, that changed:
* CHECKMULTISIGs serialized in scriptSigs are
* counted more accurately, assuming they are of the form
* ... OP_N CHECKMULTISIG ...
*/
unsigned int GetSigOpCount(bool fAccurate) const;

/**
* Accurately count sigOps, including sigOps in
...

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082414176)
This seems like a meaningful it improvement to me because it clearly states that the check is inaccurate, but if there are downsides to merging this let me know!

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082422282)
> clearly states that the check is inaccurate

Isn't that already clear by calling `bool fAccurate` with `false`? In a method called `GetLegacySigOpCount` with the doc:
```C++
/**
* Count ECDSA signature operations the old-fashioned (pre-0.6) way
* @return number of sigops this transaction's outputs will produce when spent
* @see CTransaction::FetchInputs
*/
```
? Seems to me the warnings are already all over the place.

If we need more warnings, can we improve the existing o
...

💬 achow101 commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2867783717)
ACK 1ee698fde2e8652d8eb083749edb8029c003b8db

🚀 achow101 merged a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436)

💬 sipa commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2867811571)
Posthumous Concept ACK

💬 1440000bytes commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2867822561)
> Concept NACK

This makes no difference to PR as explained by Ava in https://x.com/achow101/status/1919467263855300733?t=UoIfqeBgI-3TDEAevorOvg&s=19

⚠️ kev009 opened an issue: "WITH_DBUS shouldn't be limited to Linux"
(https://github.com/bitcoin/bitcoin/issues/32464)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

With the move to CMake, WITH_DBUS becomes a Linux only feature:
`cmake_dependent_option(WITH_DBUS "Enable DBus support." ON "CMAKE_SYSTEM_NAME STREQUAL \"Linux\" AND BUILD_GUI" OFF)`

### Expected behaviour

This should be enabled on any UNIX-like. Or alternatively disabled on WIN32.

### Steps to reproduce

cmake -DWITH_GUI -DWITH_DBUS on FreeBSD or other UNIX

### Relevant log output

_
...

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082486420)
> Or improve the code to not need so many comments in the first place?

So IIUC having the comment itself is a downside? I think different people use comments differently. I like when comments are there to help me navigate code and call out things that are notable, even repeat things that are "clear," because I find most comments easier to read than most code, and I like when I can get an overview of what is happening by just reading the comments and not even looking at the code. I think other
...

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082492235)
Yes, I usually treat comments as admitting defeat, since if we thought the code was obvious, we wouldn't feel the need to add more explanation to it.
But here it's already over-explained, what's the exact reason for wanting even more comments (but not even considering code changes like `GetSigOpCount(/*fAccurate*/false)` - which isn't just a dead comment since linters are checking it)?

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082510695)
> But here it's already over-explained, what's the exact reason for wanting even more comments

I think we probably disagree that the current code is overexplained. At least 3 of us here think that current code is confusing and a04f17a1882407db09b0a07338e12877ac1d9e92 is an improvement.

> (but not even considering code changes like `GetSigOpCount(/*fAccurate*/false)` - which isn't just a dead comment since linters are checking it)?

I didn't consider this just because I didn't know it was
...