💬 brokenprogrammer commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1428231398)
@fanquake No worries, I've squashed and updated the commit message according to the contributing guidelines.

💬 Semisol commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1428236333)
Concept NACK, at least SegWit allows for this data to be not downloaded. Alternative would be to make 256-512 byte OP_RETURNs.
Also, `OP_FALSE OP_IF OP_PUSH` is an unintended way to store data and should not be used as rationale for doing protocol changes.

✳️ fanquake pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/8126551d54ff...1ad0711d7c10)
Merge bitcoin/bitcoin#27016: mapport: require miniupnpc API version 17 or later

b3b673f7048cce1d1368819abb0b58b7c6699fa5 mapport: require miniupnpc API version 17 or later (fanquake)

Pull request description:

Version 17 is currently the latest version, see: https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/apiversions.txt, and has been available since the release of 2.1. 2.1 or newer is readily available across all distros, see https://repology.org/project/miniupnpc/versions, so drop support for the older API versions.

Split out of #22644.

ACKs for top commit:
hebasto:
ACK b3b673f7048cce1d1368819abb0b58b7c6699fa5, tested on Ubuntu 20.04 w/ and w/o [`libminiupnpc-dev`](https://packages.ubuntu.com/focal/libminiupnpc-dev) package.
TheCharlatan:
ACK b3b673f7048cce1d1368819abb0b58b7c6699fa5

Tree-SHA512: f53b36b82462c4ea83d9b83413dca8097885d1620f7ca0a53a79d6b3d3cf37c7773828b23f4278ccfcc3b14fcb0faffa35f60191b519b04570f3d2783d0303e2

💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1428240250)
@hebasto Yes. I'm going to review/test the CMake work (starting with this PR) in earnest this week. Until then, I'll hold off on further commenting/judgement.

I do stand by my comment on timing though. Getting this merged "as-is" for v25 is a bad idea imo. I suggest that we create a staging branch/repo for reviewing and merging chunks at a time, with the goal of merging from staging to master after v26 branch. Similar to the workflow (though not necessarily the timing) @sipa used for segwit.

🚀 fanquake merged a pull request: "mapport: require miniupnpc API version 17 or later"
(https://github.com/bitcoin/bitcoin/pull/27016)

💬 MarcoFalke commented on pull request "test: Fix intermittent sync issue in wallet_pruning":
(https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1428249997)
> https://cirrus-ci.com/task/5441089956478976 cry

Ah, good catch. This is the still exactly the same error as in https://github.com/bitcoin/bitcoin/issues/27065. While the fix here is unrelated to fixing that bug, I think it is still needed to fix the same bug happening in a different line.

📝 MarcoFalke opened a pull request: "test: Fix intermittent sync issue in wallet_pruning"
(https://github.com/bitcoin/bitcoin/pull/27093)
The `sync_fun=self.no_op` has no motivation or rationale, and seems to be causing issues.

Fix that by removing it.

Actually fixes https://github.com/bitcoin/bitcoin/issues/27065, see https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1428249997

💬 MarcoFalke commented on pull request "test: Fix intermittent sync issue in wallet_pruning":
(https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1428252597)
See https://github.com/bitcoin/bitcoin/pull/27093

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104733597)
ah, you are suggesting to change it to a signed type. Sure.
Pushed.

💬 TheCharlatan commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104753896)
I might have missed something, but why not use the `m_keypool_size` from the available wallet here?

```
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 64253789ec..d9c6755bcb 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -15,7 +15,6 @@
#include <script/standard.h>
#include <sync.h>
#include <util/bip32.h>
-#include <util/system.h>
#include <util/time.h>
#include <util/translation.h>
#include <wallet/rpc/util.h>
@@ -1480,7
...

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1428276879)
Updated per feedback. Thanks hebasto.

Plus added 1ff1b9c to protect `importdescriptors` from negative '-keypool' value.
Applied the same approach used in the wallet code to handle a negative keypool size. Currently, the keypool size is set to 1 in such cases, although it could also be handled by throwing an initialization error but.. the objective was to implement the smallest change in this PR to avoid extending its scope.

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104756635)
ha, because I'm still sleepy, good catch. Pushing it..

💬 instagibbs commented on issue "24.0.1 crash on restart":
(https://github.com/bitcoin/bitcoin/issues/27088#issuecomment-1428279748)
Happened againt today: https://gist.github.com/instagibbs/17d6513f26d8999ffebc25cf745d84fb

I'll try and see if I can interleaved bitcoind logs in CI as well, but it looks like at this line https://gist.github.com/instagibbs/17d6513f26d8999ffebc25cf745d84fb#file-bitcoind-crash-cln-ci-L2725 CLN thinks it talked to bitcoind fine, but immediately after it has an issue due to the crash.

💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#discussion_r1104757809)
I am still not sure what this means and which paths are preferred or discouraged

💬 john-moffett commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1428311028)
> Concept ACK, great catch indeed.

Thank you (and @furszy)!

>> Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

> I think this is the best approach. ... Additionally, I think we could also encourage the user to re-encrypt the wallet with a different passphrase (without null char) for maximum backward compatibility, and perhaps even offer a one-click solution to do so in qt?

Sounds good,
...

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104785968)
It has inside, `ReachableNets::m_mutex`.

💬 brunoerg commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104803546)
In d9b95cb1eedb9f165df93dcae0aba96ede40767a, I could check that all CJDNS address starts with [OxFC](https://github.com/cjdelisle/cjdns/blob/master/doc/Whitepaper.md#pulling-it-all-together), so, it switches the first byte to `0x55` to avoid generating it that look like CJDNS. But why specifically `0x55`?

💬 brunoerg commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104805263)
nit:
```suggestion
const bool reachable{g_reachable_nets.Contains(addr)};
```

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1428374812)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1399216846

> Not sure if related or not:

Yes, the IPC serialization code wasn't setting the `interfaces::FoundBlock.found` field, which caused wallets not to sync correctly after #26679. (The serialization bug present before #26679, but didn't have an effect until new code tried to use the missing field.)

---

Rebased 05e44e7fd41b32d57379bb6a87c4bebb4a166f9c -> 5e90c75de0d13aedb7e7c06ebe5aa55b11bb398d ([`pr/ipc.183`](https
...

💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104819743)
Note that this ends up setting:
```
C compiler ............................ /usr/bin/cc
...
C++ compiler .......................... /usr/bin/c++
```
Which is different from what autotools would find. cc/c++ are usually symlinks to a valid compiler, but they don't always exist.

For ex, this would actually break my current workflow because my current llvm+clang toolchain did not ship with these links. So I'd see surprising results with a build like:
```
export PATH=/opt/clang+llvm-14.0.
...