💬 Seccour commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834075901)
> Also removes the code to enforce those limits, including the `-datacarrier` and `-datacarriersize` config options.

Concept NACK

- Just giving up on fighting spam because they found a new way of spamming is the opposite of what we should do. We should double down and find a way to fight the spam instead of embracing it.

- Core should give bitcoiners more freedom on what rules they want to enforce, not the opposite. Removing config options is the opposite of what should be done.

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2834123418)
> All tests which tested legacy wallet behavior have been removed. The `--descriptors` and `--legacy-wallet` options are removed from the functional tests.

nit in OP, I don't think this is true. This was already done in https://github.com/bitcoin/bitcoin/pull/31250

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2834125985)
i replaced the fallback by an explicit OSError (this should be propagated to the parent through the error pipe), maybe that will help diagnose the issue.

💬 torkelrogstad commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834136354)
Concept ACK

💬 laanwj commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2834166663)
Concept ACK

💬 vasild commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#discussion_r2063129623)
The added fuzz test does assert that if addr:port equals then `CNetAddr` must also equal. Could add `Assume()` here, something like:

```cpp
bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
{
const bool match{FindNode(static_cast<CNetAddr>(addr))};
if (!match) {
Assume(!FindNode(addr.ToStringAddrPort()));
}
return match;
}
```

I think that is a kind of an overkill, given the fuzz and unit tests added, but if reviewers want it, then I will add t
...

📝 maflcko opened a pull request: " test: Force named args in RPCOverloadWrapper optional args "
(https://github.com/bitcoin/bitcoin/pull/32360)
This can avoid bugs and makes the test code easier to read, because the
order of positional args does not have to be known or assumed.

Also, contains two commits to remove dead code.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063194316)
I was referring to `if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']):`. However, I see that it may be used by tests using previous releases.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063195071)
Actually, the `cli` field can be removed. Done in https://github.com/bitcoin/bitcoin/pull/32360

🤔 janb84 reviewed a pull request: "cmake: Respect user-provided configuration-specific flags"
(https://github.com/bitcoin/bitcoin/pull/32356#pullrequestreview-2798512878)
Few suggestions, although the changes do work as described (tested)

<details>

Master:
`cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"`
```
C++ compiler flags .................... -O2 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function
...

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063214810)
```suggestion
set_property(CACHE "${var_name}" PROPERTY VALUE "${${var_name}}")
```
NIT: add quotes to ensure proper handling of variable name

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063210467)
```suggestion
string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" "${var_name}" "${${var_name}}")
```
NIT: add quotes to ensure proper handling of variable name

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063192800)
NIT: `precious_variables` is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?

💬 fanquake commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2834519435)
Backported to 29.x in #32292.

💬 fanquake commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834538457)
Not sure I understand your suggestion; are you saying we don't fix this? Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else? Note that this is a generic issue, and the PIE check is just the first check that fails in this way, if you remove the PIE check the threads check will fail the same.

💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2063248425)
Does it make sense to advance `pindexLastCommonBlock` in that case? I am just curious, it is definitely not the aim of this PR.

💬 fanquake commented on pull request "doc: Fix fuzz test_runner.py path":
(https://github.com/bitcoin/bitcoin/pull/32353#issuecomment-2834572446)
Backported to 29.x in #32292.

💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063264880)
Can you update zmq version used to 4.3.5 (https://github.com/bitcoin/bitcoin/pull/28627).

💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063265648)
Can you update sqlite version used to 3.46.1 (#29991).

💬 fanquake commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834586735)
> but was found by https://github.com/bitcoin/bitcoin/pull/32339 momentarily

It was actually first pointed out here, https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500175006, by @dergoegge.