💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860131262)
(this could also be done in 4b4d99fb2df2c60a2214487cec627bc560f50f53 `GetFanoutTargets`, although it's not that much an improvement over there)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860064472)
Multimap is ordered by key, and for same-key (same `parent_count`) since c++11 the order is consistent ([documented here](https://stackoverflow.com/a/13337612)). Feel free to include this as a comment, although I'm not even sure we care about the same exact output of multiple `SortPeersByFewestParents` calls.

(also it depends on the consistent ordering of `m_states` iterator too, which could be documented along the same lines, here and elsewhere it is iterated over)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860143301)
Yeah, and perhaps good time to squash this into eb2b19d2c237f2cc7b4ff3e7cefec52a37eb24b3, it got much stale by now.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860146357)
yes

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860154761)
The issue is re-defining `wtxid` (for `parent_wtxid`) while it's already defined as a transaction-in-question :)
That was the source in my confusion originally

💬 laanwj commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1860193767)
Yes, it's unrelated to the scope of the PR anyway.

👍 laanwj approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2464281216)
re-ACK ee6185372fc317d3948690997117e42f6b79a5ff

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1860200766)
My preference would be to drop this comment blob. Otherwise, there will be bike-shedding when it is fine to remove it. Also, the code has to be touched again at that time. It seems better to just touch it once and then be done with it.

Suggested diff:


```diff
diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
index 6a23a7b..5e18d23 100644
--- a/src/test/result_tests.cpp
+++ b/src/test/result_tests.cpp
@@ -63,7 +63,8 @@ void ExpectSuccess(const util::Result<T>& result
...

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2503243488)
ACK https://github.com/bitcoin/bitcoin/commit/32fc59796f74a2941772b5ec2755b1319132cd9c

💬 dexizer7799 commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2503389081)
Please fix the Dust/Dusting/Vector76/Double Spend Attack for Bitcoin.

💬 dexizer7799 commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2503390537)
Add support for escrow transactions.

💬 dexizer7799 commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2503397975)
Add support for choose algorithm of signing I mean support for not only Secp256k1 add Curve25519 with Ristretto.

💬 dexizer7799 commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2503405789)
Add advanced support for blind signing transactions to solve attacks like Lattice attack and other.

📝 hebasto opened a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379)
On the master branch @ 70e20ea024ce4f39abc4022e1ba19d5a6db2a207, the `APPEND_CPPFLAGS`, `APPEND_CFLAGS` and `APPEND_LDFLAGS` are not correctly applied when building C code in the `secp256k1` subtree, as intended.

This behaviour occurs due to two issues:
1. The command here: https://github.com/bitcoin/bitcoin/blob/70e20ea024ce4f39abc4022e1ba19d5a6db2a207/src/CMakeLists.txt#L77
does not affect the code in `add_subdirectory(secp256k1)` above it.

2. `APPEND_LDFLAGS` is not passed to the
...

💬 fanquake commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1860351148)
I'm not sure this is a good inline comment, because it doesn't actually document the problem, just the side-effects.

💬 fanquake commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2503431774)
> Fixes an upstream bug
> Enhances robustness

Wondering if in this case given CMakes tooling has not only been buggy, but then is also apparently not robust enough for production use without wrapping it in more layers of abstraction, maybe we should just revert to (as we did in Autotools) something like using the flags we want directly.

💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1860355374)
Mind suggesting a better wording for the comment?

💬 dergoegge commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503447959)
How can the broken behavior be observed? It appears to be working to me based on: the MSan job not spitting out false positives and coverage reports including coverage for secp.

💬 fanquake commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503455571)
> APPEND_LDFLAGS is not passed to the subtree's build system at all.

I thought #30791 fixed an issue related to passing down linker flags via `APPEND_LDFLAGS`. So has that regressed / broken since, or just never worked?

💬 maflcko commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2503468035)
review ACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1