💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1446006765)
yeah I think a temporary `set<Wtxid>` within net processing is probably the easiest

💬 theuni commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1882953986)
From what I can tell, we shouldn't need this after clang-14: https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/bitcoin_runtime_lib.m4

Is that the case?

Noticed while reviewing https://github.com/hebasto/bitcoin/pull/43/commits/910fd7a64802380fe912446cd3c938d56e470f0b#diff-b937c829d4a0cf9a653f5361656ea8196b6ecef40fd033aa41a6da945e987bcdR146

@maflcko Can we just skip porting that to CMake, or am I missing something?

🤔 stickies-v reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1811002749)
Hooray for adding descriptor wallet support. Is there a reason both legacy and descriptor wallet tests aren't in the new `wallet_rescan_unconfirmed` test, though? That seems like a more logical grouping with less duplication? If there are technical objections, I think it would be good to add some extra documentation to both tests stating that the other wallet type is tested in file xxx.py, making it easier to clean up these tests e.g. when we drop legacy wallets or make other significant changes
...

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1446005072)
Is there a reason it's 110 and not `COINBASE_MATURITY`?

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1445998556)
grammar nit
```suggestion
# Node which we will use to send funds to the wallet and mine blocks
```

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1446011145)
nit: I still think it's unnecessarily confusing for (current and future) reviewers to figure out why confirmations needs to equal 0 when afaiu this is completely unrelated to the test. I would either remove the value comparison or specifically state in the docs that we don't care about the `confirmations` value, just that the response is a JSON object with a field we recognize?

👍 shaavan approved a pull request: "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption"
(https://github.com/bitcoin/bitcoin/pull/29200#pullrequestreview-1811035999)
crACK

**Expanded Encryption Support:**
Enables I2P to connect using both ECIES-X25519 and ElGamal encryption types (4 and 0, respectively), broadening secure connectivity options beyond the default ElGamal.

**Optimal Encryption Choice:**
Code logic appropriately prioritizes the faster ECIES-X25519 encryption, aligning with best practices for security and performance.

**Clear Rationale:**
Commit message provides a concise and transparent explanation for the code update, enhancing over
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1882967719)
Yeah, I guess so. The commit were:

* https://github.com/llvm/llvm-project/commit/d0eeb64be5848a7832d13db9d69904db281d02e8
* https://github.com/llvm/llvm-project/commit/6860b136b9e14e7c5771b710bea7370d5020a94b
* https://github.com/llvm/llvm-project/commit/e9b3f2573090a2fb9494975e4615f77b898e36a3
* https://github.com/llvm/llvm-project/commit/d8b6ae072d7734e2abadd54ecddfd6cb77b7e4c0
* https://github.com/llvm/llvm-project/commit/c8c176d999d2c2166dee8d67c32b4f8f42b4f1bd
* https://github.com/l
...

💬 stickies-v commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1882971897)
I support using `contains` going forward, but I'm not sure for this PR the benefits outweigh the costs?
1. I prefer the `contains` interface but imo using `count` is also pretty straightforward, so the benefits seem limited?
2. It conflicts with a large number of PRs
3. It's not a scripted diff

I think I'd prefer removing the clang-tidy check for now and only changing it in places where it doesn't conflict with other PRs, e.g. maybe in tests? And then organically have more `contains` adopt
...

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883016117)
> Should I change this pull to bump to 14 instead, and remove the m4 file?

Why change to 14, and not just add the m4 removal to this PR?

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883019819)
> Why change to 14, and not just add the m4 removal to this PR?

Oss-Fuzz is on clang-14, so a later version can not be used, for now. See https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1878712706

📝 maflcko opened a pull request: " build: Bump clang minimum supported version to 14 "
(https://github.com/bitcoin/bitcoin/pull/29208)

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883025587)
> Oss-Fuzz is on clang-14,

I was always under the impression it was 15: https://github.com/google/oss-fuzz/blob/6c19a0f229c244d41562ece92f9baac6b72426f9/infra/base-images/base-clang/checkout_build_install_llvm.sh#L53

> OUR_LLVM_REVISION=llvmorg-15-init-1464-gbf7f8d6f

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883028960)
Looking at a recent build of a project on oss-fuzz: https://github.com/google/oss-fuzz/actions/runs/7449538792/job/20266395883#step:7:283

> -- The C compiler identification is Clang 15.0.0

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-1883029922)
That version number is bumped after the branch-off, so any commit prior to clang 15, but after clang 14, also (incorrectly) identifies as clang 15.

💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1883045460)
Concept ACK

💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1446088741)
Might have been a typo?

💬 fanquake commented on issue "new crash in v26.0":
(https://github.com/bitcoin/bitcoin/issues/29153#issuecomment-1883055919)
I assume this be moved to the GUI repo? @hebasto

💬 fanquake commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1446092071)
@theuni If you want I can also just include a fix for this in here, while we are sorting out flags.

💬 furszy commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1883057125)
> I think the error message "Unable to parse settings file %s" could definitely be improved though. Would suggest maybe "Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, and can be fixed by removing the file, which will reset settings to default values." This would probably work OK for both command line and GUI (since the GUI already offers the option to reset the settings see [bitcoin-core/gui#379](https://github.com/bitcoin-core/gui/pull/379)
...