💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825893170)
Happy to consider an alternative approach that allows us to check there are no collisions without having to expose a public method if you have something in mind

⚠️ pinheadmz opened an issue: "Source code mapping for debugger has changed since cmake"
(https://github.com/bitcoin/bitcoin/issues/31204)
developer-notes.md says:


>1. Configure source file mapping.
>
>For `gdb` create or append to `.gdbinit` file:
>```
>set substitute-path ./src /path/to/project/root/src
>```
>
>For `lldb` create or append to `.lldbinit` file:
>```
>settings set target.source-map ./src /path/to/project/root/src
>```

But I found I needed to create a `.lldbinit` file with these lines:
```
settings append target.source-map build/src/test/ /Users/matthewzipkin/Desktop/work/bitcoin/
settings append
...

💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451988229)
> this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing.

I don't think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build (with different build options) and expect it to work, or be a supported use-case. This has also been the case up until two weeks ago, before commit 9f243cd7fa6654e3b71ba6bff82cceed547c5d
...

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825901123)
Ok, given new interface seems impossible to hit. Added another test case to `ComapreChunks` to just make sure it was well-defined:

```
diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
index c6d06c937c..d610661878 100644
--- a/src/test/rbf_tests.cpp
+++ b/src/test/rbf_tests.cpp
@@ -631,20 +631,32 @@ BOOST_AUTO_TEST_CASE(feerate_chunks_utilities)
{
// Sanity check the correctness of the feerate chunks comparison.

// A strictly better case.
std::vector<Fee
...

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825906517)
history is cleaner, thank you

💬 maflcko commented on pull request "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction":
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2452007044)
lgtm ACK 5a26cf7773efdc316ef8235c541ad39e230cd10a

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825913918)
I don't think this comment is accurate, since this can never be the case. A transaction considered for relay cannot have in-mempool descendants by definition, since they would have been orphans at the time of considering them.

I cannot recall if this is a re-word of the previous approach, which tried to consider descendants, or I tried to refer to the collisions and it did;t make much sense.

I'll remove the comment

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2452011156)
ACK 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf

via `git range-diff master e2324779781b695024d3b17340001fa2da5d3c7f 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf`

💬 hebasto commented on pull request "depends: bump boost to 1.86.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2452015248)
FWIW, Debian provides `boost_headers-config.cmake`:
- https://packages.debian.org/bookworm/amd64/libboost1.81-dev/filelist
- https://packages.debian.org/trixie/amd64/libboost1.83-dev/filelist

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825925061)
Done.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825925932)
Maybe worth a separate PR to ensure the test coverage.

💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452025672)
This pull: https://github.com/bitcoin/bitcoin/actions/runs/11630801646/job/32390526596?pr=31187#step:6:337

```
C++ compiler flags .................... -Wno-error=unused-member-function -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/home/runner/work/bitcoin/bitcoin=. -fmacro-prefix-map=/home/runner/work/bitcoin/bitcoin=. -Werror -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-
...

💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452026337)
lgtm ACK 54d07dd37d5919c4e3bc535ae498d623282741d1

💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py AssertionError: [node 2] Node returned unexpected exit code (1) vs (0) when stopping":
(https://github.com/bitcoin/bitcoin/issues/30798#issuecomment-2452045667)
https://cirrus-ci.com/task/6288622511456256?logs=ci#L3303

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825939865)
That's a good point. It may be the case that the selected peer has a parent -> child dependency but was not picked as one of the `fanout_with_ancestors` peers, the parent and the child may have ended up in different sets.

We should check the return of `TryRemovingFromSet` before adding it to `vInv`

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825957496)
These collisions of this kind *could* happen in theory, but I don't think they can be really gamed:

short ids are peer specific, they are generated using `SipHashUint256` which are salted using ` m_k0` and `m_k1`. Therefore, while a collision of this kind is theoretically possible, an adversarial peer should not be able to guess collisions and hence exploit them

💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1825957996)
Re https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824164226: I think I agree with marco that a compile time check would be nicer and we should ideally upstream it so that our `FuzzedDataProvider` impl stays in sync.

👍 dergoegge approved a pull request: "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction"
(https://github.com/bitcoin/bitcoin/pull/31203#pullrequestreview-2410304438)
utACK 5a26cf7773efdc316ef8235c541ad39e230cd10a

💬 sr-gi commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2452082143)
Removed dummy commit

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825965609)
Yes, that seems better. Making ProcessPCP a void is also fine with me.