💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746)
> Conditional calls to AC_DEFINE (such as those setting it to a constant `1`, or not setting a value at all) need `defined()`/`#ifdef` AIUI

Agree with @luke-jr.

In this particular case, it is conditional: https://github.com/bitcoin/bitcoin/blob/8126551d54ffd290ed5767248be4b3d19243787b/configure.ac#L1707-L1710

therefore, the current code is fine.

💬 jonatack commented on pull request "cli: include local ("unroutable") peers in -netinfo table":
(https://github.com/bitcoin/bitcoin/pull/26584#issuecomment-1428125914)
This is not a risky change and seems fine to merge.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104628124)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104282294

> Allowing for a `void` failure type seems to make this incompatible to be switched out to `std::expected` https://en.cppreference.com/w/cpp/utility/expected ?
>
> With multiple warning and error messages this may already be incompatible, though?

Yes, if you are thinking that `util::Result` could wrap `std::expected` that would be probably be awkward and not worth it.

But I don't think there is a conflict becaus
...

💬 brunoerg commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#discussion_r1104630727)
Since you're replacing `base64` lib to use `secrets`, perhaps would be good to do the same in `rpcauth-test`?

💬 hebasto commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104633385)
See:
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#es101-use-unsigned-types-for-bit-manipulation
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#es102-use-signed-types-for-arithmetic
- [Signed and Unsigned Types in Interfaces](https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf)

Suggesting:
```suggestion
int64_t m_keypool_size{DEFAULT_KEYPOOL_SIZE};
```

💬 TysonsTech commented on pull request "Minor edits - punctuation, spelling to make the contributing instructions more readable for all.":
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1104634402)
Appreciate the feedback, and the helpful article links! Thank you, sir!

👍 TheCharlatan approved a pull request: "mapport: require miniupnpc API version 17 or later"
(https://github.com/bitcoin/bitcoin/pull/27016)

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-1428171645)
> we don't end up with any foritfied funcs in bitcoin-util or bitcoin-cli.

`bitcoin-cli` is fixed once we fortify libevent.

💬 mzumsande commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1103877367)
Does this need a Mutex, considering it's being accessed from both Node and GUI?

💬 mzumsande commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104641668)
Now that `MaybeFlipIPv6toCJDNS` is available in `netbase`, would it make sense (maybe in follow-ups) to also use it in the `Lookup(` functions, not just `LookupSubNet` and reduce the number of occurrences we have to call `MaybeFlipIPv6toCJDNS` from outside it?

💬 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
...