💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2293235155)
nit: Maybe update/remove year? (Not a copyright lawyer).

💬 hodlinator commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2293305352)
Could assert that various conditions are true:
```suggestion
virtual ~SockMan()
{
assert(!m_thread_socket_handler.joinable()); // Missing call to JoinSocketsThreads()
assert(m_connected.empty()); // Missing call to CloseConnection()
assert(m_listen.empty()); // Missing call to StopListening()
}
```
https://en.cppreference.com/w/cpp/thread/thread/join.html#Postconditions

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300508652)
the todo refers to the value of `m_pay_tx_fee`, not to the hardcoded `0` here. Also, see https://github.com/bitcoin/bitcoin/pull/32138/files

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#discussion_r2300510188)
this is just removing magic values in one place and adding them in another. what is the point?

💬 maflcko commented on pull request "wallet: Replace fee magic numbers with named constants":
(https://github.com/bitcoin/bitcoin/pull/33254#issuecomment-3223533891)
Thanks, but the value of purely LLM generated "vibe" pull requests is limited, because:

* The "author" does not understand the changes and can not reply to code review feedback.
* There are more than 300 pull requests (most written by real humans) waiting for review.
* Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.

My recommendati
...

✅ maflcko closed a pull request: "wallet: Replace fee magic numbers with named constants"
(https://github.com/bitcoin/bitcoin/pull/33254)

💬 maflcko commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3223540909)
I think this was fixed and can be closed?

✅ maflcko closed a pull request: "doc: add Linux GUI runtime instructions to doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/33197)

💬 maflcko commented on pull request "doc: add Linux GUI runtime instructions to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/33197#issuecomment-3223543034)
Closing for now, as this was fixed already.

💬 ryanofsky commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3223640756)
From https://ubuntu.com/security/CVE-2022-46149 it seems to say ubuntu has looked at the issue and ignoring it in LTS releases ("Ignored" value in Status table), because the fix changes [changes the ABI](https://github.com/capnproto/capnproto/compare/v0.8.0..v0.8.1#diff-22b74c4b9a5b4d48aece4e5b3b4e73a0a680be156c8921c1d135c1c1cac3d8d1R1235) and could maybe break applications compiled against the 0.8.0 version of the headers but using the 0.8.1 runtime library (IIUC, I'm not exactly sure what they
...

⚠️ fanquake opened an issue: "ci: tidy job is producing output"
(https://github.com/bitcoin/bitcoin/issues/33256)
master (6ca6f3b37b992591726bd13b494369bee3bd6468) https://cirrus-ci.com/task/5658586602274816:
```bash
[17:17:19.943] [108/715][22.0s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/echo.capnp.proxy-server.c++
[17:17:34.032] /usr/include/kj/async-inl.h:609:37: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorRes
...

💬 fanquake commented on issue "ci: tidy job is producing output":
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3223652850)
cc @Sjors @ryanofsky

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620446)
Yeah, encoding seems like another possible source for inconsistencies. I don't think we want to deal with encoding here, because the value is small enough for it to not matter.

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620527)
thx, reworded the comment a bit

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620620)
It is mostly a regression test, checking that on master a single arg with size of `CLI_MAX_ARG_SIZE = 131071` fails the test (with the reproducer shared in the pull description).

Force pushed to reduce to that (and added a comment).

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2300620722)
> This test also passes on master (MacOS environment with ARG_MAX = 1048576).

Thanks for adding a macos data point for reference. This further supports that the hard-coded value may be different on different platforms.

> However, the following test only passes on this PR, not on my master:

Nice, thanks for adapting the test and checking the bugfix on macos.

💬 maflcko commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3223742148)
Seems fine to pick this up now, if you want.

💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300759162)
Just an fyi, this test passes even without the changes in `src/policy/fees.h` I also agree with @polespinasa that if another test already covers this, then I'd rather not have redundant tests added

💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2300768342)
I am not sure if we need a release note, since according to this note, it will invalidate old estimates files? Maybe someone else can chime in on this

💬 kevkevinpal commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3223886285)
Concept ACK [413c81b](https://github.com/bitcoin/bitcoin/pull/33199/commits/413c81b134a16403cc0f64e21c3c0967c211b318)

It makes sense to update `MIN_BUCKET_FEERATE` with the recent changes to be sub 1 sat/vbyte

I do think the additional test is a bit redundant but I am okay in adding if others think it makes sense