💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379723181)
This is certainly better. For no good reason, i just chose to follow a pattern we use elsewhere and never reconsidered it. I will take this code.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1379749369)
I'm always not sure what to do with these kinds of probabilistic scenarios... Say you run 1000 experiments, and get 1000/0, so the assert fails. Is 1,000,000 sufficient in that case? Or how do you think this should be asserted otherwise.

💬 maflcko commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1379759963)
I agree, but I don't think there is an easy and recommended way to disable a single warning to not error. I could embed it into the compiler (`CXX='clang++ -Wno-error=deprecated-declarations`), but I presume this will be overridden by the configure logic to enable errors. I could set it in `CXXFLAGS`, but I presume this will either be overridden in the configure logic, or it will cause other horrible side-effects, such as disabling O2 (https://github.com/bitcoin/bitcoin/pull/28071/files).
Final
...

💬 maflcko commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1790306232)
Is it still accurate to call the coverage report `test_bitcoin` when it may cover something else?

Do the docs need an update? https://github.com/bitcoin/bitcoin/blob/eca2e430acf50f11da2220f56d13e20073a57c9b/doc/developer-notes.md?plain=1#L478 etc ...

💬 maflcko commented on pull request "tests: Fix LCOV_OPTS to be in the correct position":
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1790311187)
Can you clarify whether this has any visible effect on the generated html? I presume only when branch coverage is enabled?

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1379813340)
Er, I'm pretty sure two transactions with the same virtual size but different amounts of witness data are treated exactly the same when checking against the maximum weight. These are both 2000 weight units.

💬 fanquake commented on pull request "build: Update `qt` package up to 5.15.11":
(https://github.com/bitcoin/bitcoin/pull/28769#issuecomment-1790387863)
Guix Build (x86_64):
```bash
f3d6ac10842f48884b36d704cc4303ee001197a5c39f7d65116324ab919f4497 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/SHA256SUMS.part
a651622edbca1c9badf7f4018296639929f9553c257bc2cdb464c240e5ab29d2 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/bitcoin-8047bb6feaa9-aarch64-linux-gnu-debug.tar.gz
6ad8004e3739c51380032a411f43054a7768188033bcd9534ebb165b1755819d guix-build-8047bb6feaa9/output/aarch64-linux-gnu/bitcoin-8047bb6feaa9-aarch64-linux-gnu.tar.gz
aaecf84
...

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1379832870)
This is also not an issue because we omit the already-in-mempool transactions in our linearization.

🚀 fanquake merged a pull request: "refactor: Remove unused circular include dependency from validation.cpp"
(https://github.com/bitcoin/bitcoin/pull/28770)

💬 fanquake commented on pull request "build: Update `qt` package up to 5.15.11":
(https://github.com/bitcoin/bitcoin/pull/28769#issuecomment-1790401466)
Couldn't see any patches that we can drop.

🚀 fanquake merged a pull request: "build: Update `qt` package up to 5.15.11"
(https://github.com/bitcoin/bitcoin/pull/28769)

💬 hebasto commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1790406668)
```
$ make -C depends capnp MULTIPROCESS=1 HOST=x86_64-w64-mingw32
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Building capnp...
make[1]: Entering directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
make all-am
make[2]: Entering directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
/bin/bash ./libtool --tag=CXX --mode=link x86_64-w64-mingw32-g++-posix -I./src -I./src -DKJ_HEA
...

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1379847330)
The idea here is just to make sure a descendant doesn't come before its child. If A is an ancestor of B, then all of A's ancestors are also B's ancestors. It must be that A.ancestor_count < B.ancestor_count. Using total ancestor vsize would also work, but count is much easier to check.

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1379853153)
If the sorting is broken, it means that `FilteredTxns` etc. might return transactions out of order, but this means validation will get a "missing inputs" error and nothing will be submitted. It can also mean that `FilteredTxns` doesn't give the more incentive compatible transactions first, which can affect whether we accept or reject these transactions.

This function being incorrect can't make us produce a consensus-invalid block template. Transactions still need to pass validation to be subm
...

💬 fanquake commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1790453392)
While we are changing this, could also add the following. I assume we don't use the TLS lib for anything, so we can skip the configure checks, and building the library if OpenSSL happens to be found:
```diff
--- a/depends/packages/native_capnp.mk
+++ b/depends/packages/native_capnp.mk
@@ -5,6 +5,10 @@ $(package)_download_file=capnproto-c++-$($(package)_version).tar.gz
$(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz
$(package)_sha256_hash=0f7f4b8a76a2cdb284fddef20de8306450
...

📝 maflcko opened a pull request: "refactor: Remove unused circular include dependency from kernel/coinstats.cpp"
(https://github.com/bitcoin/bitcoin/pull/28773)
Also, iwyu

💬 fanquake commented on pull request "refactor: Remove unused circular include dependency from kernel/coinstats.cpp":
(https://github.com/bitcoin/bitcoin/pull/28773#issuecomment-1790469398)
https://github.com/bitcoin/bitcoin/pull/28773/checks?check_run_id=18295730879:
> A new circular dependency in the form of "kernel/coinstats -> node/blockstorage -> validation -> kernel/coinstats" appears to have been introduced.

💬 maflcko commented on pull request "refactor: Remove unused circular include dependency from kernel/coinstats.cpp":
(https://github.com/bitcoin/bitcoin/pull/28773#issuecomment-1790492880)
> A new circular dependency in the form of "kernel/coinstats -> node/blockstorage -> validation -> kernel/coinstats" appears to have been introduced.

Yeah, this is identical to, and already tracked as `"node/blockstorage -> validation -> node/blockstorage"`. I guess I can close for now, until the other one is fixed.

✅ maflcko closed a pull request: "refactor: Remove unused circular include dependency from kernel/coinstats.cpp"
(https://github.com/bitcoin/bitcoin/pull/28773)

💬 fanquake commented on pull request "refactor: [tidy] modernize-type-traits":
(https://github.com/bitcoin/bitcoin/pull/28664#issuecomment-1790517127)
Might come back to this when bear/tidy are working.