💬 MarcoFalke commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428093567)
> we still want the P2P network to obscure transaction relay beyond that

I wonder if that is worth it. Given this issue here (and past ones), it just seems hard to think about and any guarantees are at best brittle in an evolving P2P network. So, long term, assuming the private "first mile" privacy-preserving fan out stuff is available, users and wallets caring about it will probably use that. Attempts to optimize the normal relay to be equally privacy-preserving will always have a taste of a
...

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1428104313)
> Does/can this also close #16220?

I will check this out, specifically w.r.t this comment which I had not been considering:

> It would be, however the reason that wallets/ is not created when the .bitcoin directory already exists is backwards compatibility. If there is an existing datadir but no wallets directory it's assumed to be an old data directory, and creating wallets would effectively hide the existing wallets in the top level dir, possibly leading to people to suspect loss of fund
...

💬 jonatack 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_r1103668241)
This change is incorrect.

💬 jonatack 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_r1103668412)
Incorrect change.

💬 jonatack 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_r1103668222)
This incorrectly changes the intended meaning.

💬 sipa commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428116350)
We can't rely on Tor for all wallet privacy, especially given that it's a centralized service that might just fail completely one day (and before that, it's hard to bound how much sufficiently powerful attackers can learn from traffic analysis in Tor).

Privacy on a public network is always multi-faceted, and it's fair we can't make sure many guarantees. But on the other hand, we go through pretty substantial efforts to hide *lots* of things on a best-effort basis, especially involving transac
...

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