💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161064917)
You already have your own CI setup and build methods, but they're like using a cannon to kill a fly. I believe the official repository should include a simple way to run Bitcoin Core without worrying about the operating system, dependencies, or anything like that — everything could be handled through a Dockerfile.

As I mentioned, people already use forks of Bitcoin Core like Umbrel’s or Start9’s, which include Dockerfiles. People install Bitcoin Core like any other app, and it just magically
...

💬 sipa commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3161082300)
Concept ACK

🤔 murchandamus reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-3093620724)
code review, lightly tested ACK d3b8a54a81209420ef6447dd4581e1b6b8550647

> * Passing negative virtual bytes to `GetFee` will return -1 as the fee.

From experimenting a bit, it seems to me that before this PR, a negative vsize would wrap from max value, i.e. `GetFee(<negative>)` would return huge amounts (here with a feerate of 1000 s/kvB):

```
./test/amount_tests.cpp(40): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(-1) == CAmount(-1) has failed [4294967295 != -1]
./test/
...

💬 murchandamus commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2257860390)
I was curious whether this test would have passed prior to this change, because the vsize was then expected to be an unsigned integer, and it does.

💬 l0rinc commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161213864)
Rebased, tested it with:
```patch
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt (revision 27e4b8c6856194fa8db028b6f7356f83ea3d7e3a)
+++ b/CMakeLists.txt (date 1754504896922)
@@ -186,6 +186,7 @@
string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")

set(configure_warnings)
+list(APPEND configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144")

include(CheckLinkerSupportsPIE)
check_linker_supports_pie(configure_warnings)
```

running
...

💬 optout21 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161302880)
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.

I understand the `== 1`/`!= 1` parts, but for `< 1`: it means that the value can be only 0, which means "doesn't contain", so it applies equally to de-duplicated containers (map, set) and duplicated containers as well, isn't it?

💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161354674)
Yes, applies to any kind of map, but I had to find and change those manually - clang-tidy doesn't detect or fix those.

💬 optout21 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3161358887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb

- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only

(I've reviewed the proposed changed, but didn't look for other potential changes)

🤔 optout21 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094#pullrequestreview-3093962887)
ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb

- The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
- It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
- Changes are localized (each to a single line), local impact only

(I've reviewed the proposed changed, but didn't look for other potential changes)

🤔 glozow reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3093839590)
ACK 43a479ca688, only minor nits

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258108951)
Could make this version 2 to be explicit

Also, could make this more efficient by reusing the outputs vector

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258016408)
Is this supposed to say "we are spending an unconfirmed TRUC transaction" ? Also, can be one line?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258128435)
nit: These last 2 subtests don't have a log associated with them

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258112001)
```suggestion
def va_tx_spends_confirmed_vb_tx(self, version_a, version_b):
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258125275)
nit: this format seems to be an outlier

```suggestion
self.log.info("Test setting version to 3 with send")
```

💬 Sammie05 commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3161405159)
Nice work on this! The label-based trigger for PeriodicMergeCICheck is a clean approach.

One thought though since this is driven by label events, do we need to consider cases where multiple labels are applied at once? Would the condition still behave as expected?

Also agree with the suggestion from @maflcko having a vector of task names could as well improve flexibility

👍 pinheadmz approved a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-3093848042)
ACK 0a9a194078

Built and tested on macos/arm64, reviewed each commit. Adds a unit test for node initialization, refactors IPC on some upstream changes in libmultiprocess, and ensures a clean shutdown when IPC clients are connected. I'm going to locally rebase https://github.com/bitcoin/bitcoin/pull/32297 on this and [try to break it](https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-3074628613) again but I suspect it'll hold up a lot better.

💬 pinheadmz commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258021983)
637502894db32a1d532601ecc70d18f9b8a35f19

nit: comment above should stay attached to line below

💬 pinheadmz commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258088297)
637502894db32a1d532601ecc70d18f9b8a35f19

Confirmed the test failure by removing these two lines, but that kinda surprised me because I assumed the test environment gets reset between modules anyway...?

```
Assertion failed: (m_buffering), function StartLogging, file logging.cpp, line 56.
```

💬 pinheadmz commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258111912)
ff1c9278e7aee8be094c63e90ca37b644f55cc7a

If there is no parent connection does that mean for sure there are no other incoming connections? The implication on line 68 is that the first incoming connection is always the parent process which skips its removal.