💬 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 ...
(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?
(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.
(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
...
(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.
(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)
(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.
(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)
(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
...
(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.
(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
...
(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
...
(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
(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.
(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.
(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)
(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.
(https://github.com/bitcoin/bitcoin/pull/28664#issuecomment-1790517127)
Might come back to this when bear/tidy are working.
✅ fanquake closed a pull request: "refactor: [tidy] modernize-type-traits"
(https://github.com/bitcoin/bitcoin/pull/28664)
(https://github.com/bitcoin/bitcoin/pull/28664)
🤔 glozow reviewed a pull request: "tests, bug fix: DisconnectedBlockTransactions rewrite followups"
(https://github.com/bitcoin/bitcoin/pull/28530#pullrequestreview-1709976459)
ACK 9b3da70bd06b45482e7211aa95637a72bd115553
(https://github.com/bitcoin/bitcoin/pull/28530#pullrequestreview-1709976459)
ACK 9b3da70bd06b45482e7211aa95637a72bd115553
💬 glozow commented on pull request "tests, bug fix: DisconnectedBlockTransactions rewrite followups":
(https://github.com/bitcoin/bitcoin/pull/28530#discussion_r1379932340)
For future reference for b2d04479647af64ad7cf5ebfb6175251b2f6b72e
Imagining `DynamicMemoryUsage` for a `DisconnectedBlockTransactions` with 1 transaction (ignoring the `iters_by_txid` part since that doesn't change),
before
```
DynamicUsage(queuedTx) + cachedInnerUsage
=MallocUsage(sizeof(list_node<CTransactionRef>) + cachedInnerUsage
=MallocUsage(sizeof(4 pointers) + RecursiveDynamicUsage(CTransaction)
=MallocUsage(sizeof(4 pointers) + DynamicUsage(vin) + DynamicUsage(vout) + sum([R
...
(https://github.com/bitcoin/bitcoin/pull/28530#discussion_r1379932340)
For future reference for b2d04479647af64ad7cf5ebfb6175251b2f6b72e
Imagining `DynamicMemoryUsage` for a `DisconnectedBlockTransactions` with 1 transaction (ignoring the `iters_by_txid` part since that doesn't change),
before
```
DynamicUsage(queuedTx) + cachedInnerUsage
=MallocUsage(sizeof(list_node<CTransactionRef>) + cachedInnerUsage
=MallocUsage(sizeof(4 pointers) + RecursiveDynamicUsage(CTransaction)
=MallocUsage(sizeof(4 pointers) + DynamicUsage(vin) + DynamicUsage(vout) + sum([R
...