💬 l0rinc commented on pull request "leveldb: show non-default options during init":
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-3404295703)
Thanks for the hint @pablomartin4btc, I kept the original line and added and extra debug log, when starting with `-debug=leveldb`.

💬 maflcko commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3404776272)
No objection, but my worry is that the error is not exactly trivial to understand and actionable:

* In https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8 it was fixed by changing the linker. While the patch looks harmless and is easy to test to fix the issue, I don't think it is straightforward to see why a runtime sanitizer issue is addressed by link-time changes.
* In https://github.com/bitcoin/bitcoin/pull/29676#issuecomment-2007701379 it was fixed by accid
...

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3404864601)
Thank you @glozow and @instagibbs for the review.

Addressed feedback:
- Removed the `create_empty_fork` method per @instagibbs's [comment](https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3402220995)
- Modified changes in the next commit as suggested by @glozow ([comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430396234))
- Made `FORK_LENGTH` a default argument per @glozow's [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430401045)
- Rem
...

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#issuecomment-3404866372)
`1197919eab...07a926474b`: address some suggestions

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431349116)
Done.

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431352462)
Done. Also shortened the names a little bit because now values have to be prefixed by the enum name.

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431356881)
It isn't a C++ method, but is a [method](https://www.merriam-webster.com/dictionary/method), "a way, technique, or process of or for doing something".

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431362561)
To make sure that if a new enum value is added it will be handled properly - this snippet will be updated. If it is forgotten, then there will be a compiler warning about unhandled enum value.

💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431365052)
I prefer to avoid duplicating `WalletLogPrintf("Submitting wtx...", wtx.GetHash().ToString());`

🤔 stratospher reviewed a pull request: "test: add functional test for `TestShell` (matching doc example)"
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3338732396)
ACK 57f7c68.

💬 stratospher commented on pull request "test: add functional test for `TestShell` (matching doc example)":
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2431376770)
57f7c68: nit: would keeping it in something like `TEST_FRAMEWORK_TESTSHELL_TESTS` (similar to `TEST_FRAMEWORK_UNIT_TESTS` be a better place) since it's a test framework infra test.

💬 maflcko commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3405047054)
re-lgtm ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

Only change seems to be a comment/doc.

🤔 janb84 reviewed a pull request: "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`"
(https://github.com/bitcoin/bitcoin/pull/33624#pullrequestreview-3338930586)
tested ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

PR introduces new assert in test that prohibits the `if ( 1 == 1)` mutation / `if(true)`, because the assert will fail in that condition.

- code review ✅
- build & tested ✅
- changed code to true to test assert ✅

🤔 maflcko reviewed a pull request: "init: Improve -asmap option behavior and documentation"
(https://github.com/bitcoin/bitcoin/pull/33632#pullrequestreview-3338942228)
(nit from the llm bot)

💬 maflcko commented on pull request "init: Improve -asmap option behavior and documentation":
(https://github.com/bitcoin/bitcoin/pull/33632#discussion_r2431523908)

Specify asn mapping used for bucketing of the peers -> Specify the ASN mapping used for bucketing of peers [adds missing article and capitalizes the ASN acronym to improve clarity]

💬 hodlinator commented on pull request "http: Make server shutdown more robust":
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-3405114414)
Was worried that libevent would clean up the memory leak I thought was fixed by aa11ee5ccfcedf3fb44e7722ecdeb3cdaa92fd51 later during shutdown, so verified that that libevent does no such thing using Valgrind: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2025/02/stop_http_robust.valgrind

💬 willcl-ark commented on issue "`test_bitcoin-qt`: segfault under LTO (CMAKE_INTERPROCEDURAL_OPTIMIZATION=ON)":
(https://github.com/bitcoin/bitcoin/issues/33548#issuecomment-3405201314)
FWIW this doesn't reproduce for me using Clang 20 via [this nix flake](https://github.com/bitcoin-dev-tools/bix/blob/wip-updates/flake.nix), where I need to force `QT_QPA_PLATFORM=xcb` to run Qt:

<details>
<summary>Details</summary>

```
Configure summary
=================
Executables:
bitcoin ............................. ON
bitcoind ............................ ON
bitcoin-node (multiprocess) ......... OFF
bitcoin-qt (GUI) .................... ON
bitcoin-gui (GUI, multiprocess) .....
...

🚀 fanquake merged a pull request: "ci: add libcpp hardening flags to macOS fuzz job"
(https://github.com/bitcoin/bitcoin/pull/33462)

🚀 fanquake merged a pull request: "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`"
(https://github.com/bitcoin/bitcoin/pull/33624)

💬 fanquake commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3405473825)
> my worry is that the error is not exactly trivial to understand and actionable:

I agree that it's odd, and I'm wondering why it's not happening (seemingly at all?) in the qa-assets repo?