🤔 mzumsande reviewed a pull request: "chainparams: Remove seed.bitcoinstats.com"
(https://github.com/bitcoin/bitcoin/pull/30720#pullrequestreview-2262327212)
ACK c88a7dc53e3be7489605c3326cf768df5437393a

It can be re-added when it's fixed.

💬 mzumsande commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2311729974)
> You mean if you invalidateblock, then reconsiderblock, the rest of the child blocks also need to be reconsidered individually?

That's already happening, reconsiderblock already goes over the entire block index db on master, looking at each index individually, see `Chainstate::ResetBlockFailureFlags` (this also has a bug, #30479, but that is unrelated to the failure flags).

💬 maflcko commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2311750723)
> I think I might not have enough information to understand the weak nack

It is the line prior: "Seems odd to change something twice, when it can be changed once? weak NACK for now."

Now that the other pull is closed, I guess this is fine.

💬 maflcko commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2311759659)
lgtm ACK b7aae361b273f2f439d3b278214b7e37908c8cb0

Seems fine to change the documentation here to be a description of what the code does right now. Also, it seems fine for this description to be applied going forward, because both an "Error" or "Warning" should be considered an Alert for the node admin to investigate (and possibly dismiss) (with or without the node shutting down).

There are probably cases where an Error can be "downgraded" to a Warning in the code, but they can be done in f
...

💬 hodlinator commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2311773405)
Thanks, I should have searched for why the -342 check was introduced before I suggested removing it. :facepalm: And my git-fu could do with some leveling up, that's impressive how it can single out the right commit.

Updated my [suggested diff](https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2311170636), bringing back the -342 check.

> As for the ignored_errors counter, I presume it will just count errno.ECONNREFUSED for WinError 10061?

Yes, that's what [ConnectionRefusedErr
...

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1732338260)
I added it just to make the `updateRwSetting` method easier to read. I will update when their is need to retouch.

💬 hodlinator commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2311831456)
> Other than that the patch
>
> > Seen recently here (`wallet_bumpfee.py --descriptors`) https://github.com/bitcoin/bitcoin/actions/runs/9863555844/job/27236587622#step:27:12496:
>
> This one looks different, because `node1` never actually starts to start up. It doesn't even get to emit an `10061` error.

The linked log has:
```
node1 2024-07-09T20:27:21.198248Z [shutoff] [D:\a\bitcoin\bitcoin\src\init.cpp:386] [Shutdown] Shutdown: done
test 2024-07-09T20:27:21.233000Z TestFramewo
...

💬 ismaelsadeeq commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2311832660)
> Any reason not to just lock the mutex across the three steps?

Yes the reasons where highlighted in https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2258765600 and https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2260433636 comments

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2311866895)
> doc/developer-notes.md needs to be updated too.

@l0rinc has already started to work on it in https://github.com/hebasto/bitcoin/pull/337. It will be converted in a follow up PR once this PR is merged.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1732369832)
Correct. Will be updated in a follow up PR, along with a few other amendments such as https://github.com/bitcoin/bitcoin/pull/30712.

🤔 glozow reviewed a pull request: "Pre-28.x branch off version bump and doc updates"
(https://github.com/bitcoin/bitcoin/pull/30719#pullrequestreview-2262535007)
lgtm, should also do doc/release-notes/release-notes-27064.md

💬 TheCharlatan commented on pull request "bench: [refactor] iwyu":
(https://github.com/bitcoin/bitcoin/pull/30716#issuecomment-2311898883)
Still seems to be:
```
bench/bench.h should add these lines:
#include "nanobench.h" // for Bench (ptr only)

bench/bench.h should remove these lines:

The full include-list for bench/bench.h:
#include <bench/nanobench.h>
#include <util/fs.h> // for path
#include <util/macros.h> // for PASTE2, STRINGIZE
#include <chrono> // for milliseconds
#include <cstdint> // for uint8_t
#include <functional> // for function
#include <map> // for map
#inclu
...

💬 ismaelsadeeq commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1732382841)
nit: add spaces between passed arguments
```suggestion
tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=script_pub_key, amount=int(amount * COIN))
```

💬 ismaelsadeeq commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1732394215)
8c64048c443cddce201b6d54184b32f3894ab806 can be squashed together with the first commit

💬 ismaelsadeeq commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1732387713)
It might be better to return a tuple here
```suggestion
return tx["txid"], tx["sent_vout"]
```

💬 ismaelsadeeq commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1732385781)
Why are you converting the amount to an integer here? Won’t that change the assumptions in the test? I see 49.999 explicitly passed in the test—don’t we want to maintain precision?

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2311917823)
> I was surprised Qt wasn't at least enabled by default.

The default build options were tailored to meet the needs of the majority of this repository's contributors.

> > The "auto" value is no longer available; non-default values must be specified explicitly.
>
> I assume this is a CMake standard practice?

It is.

> Or is there some other reason to not autodetect bdb/portmapping/qt/zmq?

Package autodetection can introduce unsolicited changes in the build configuration, which is
...

💬 hodlinator commented on pull request "bench: [refactor] iwyu":
(https://github.com/bitcoin/bitcoin/pull/30716#issuecomment-2311922032)
Concept ACK

Thanks for taking the initiative on this! (Should hopefully free contributors from considering `#include`s when reviewing, recent example of me doing so: https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2253654936).

💬 glozow commented on pull request "chainparams: Remove seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/30720#issuecomment-2311926209)
lgtm looking at discussion on #29911

cc @cdecker

💬 cdecker commented on pull request "chainparams: Remove seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/30720#issuecomment-2311931636)
I'm in the process of fixing the seeder. It should be back online by the end of the week, if that's acceptable