🚀 fanquake merged a pull request: "doc: remove mention of missing bdb being a configure error"
(https://github.com/bitcoin/bitcoin/pull/28881)
(https://github.com/bitcoin/bitcoin/pull/28881)
🚀 fanquake merged a pull request: "doc: remove x86_64 build assumption from depends doc"
(https://github.com/bitcoin/bitcoin/pull/28884)
(https://github.com/bitcoin/bitcoin/pull/28884)
🚀 fanquake merged a pull request: "bench: Update nanobench to 4.3.11"
(https://github.com/bitcoin/bitcoin/pull/28877)
(https://github.com/bitcoin/bitcoin/pull/28877)
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1814117821)
`2386ef0a54...0858b0c084`: remove duplicate `#include <sync.h>`
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1814117821)
`2386ef0a54...0858b0c084`: remove duplicate `#include <sync.h>`
💬 hf5pro commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1814119792)
> The problem that RBF is trying to solve could simply be solved by increasing the acceptable blockweight.
Folks over at BSV are testing big blocks, may want take a look ;)
https://github.com/bitcoin-sv/bitcoin-sv
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1814119792)
> The problem that RBF is trying to solve could simply be solved by increasing the acceptable blockweight.
Folks over at BSV are testing big blocks, may want take a look ;)
https://github.com/bitcoin-sv/bitcoin-sv
👍 fanquake approved a pull request: "tests: Fix LCOV_OPTS to be in the correct position"
(https://github.com/bitcoin/bitcoin/pull/28771#pullrequestreview-1734005073)
ACK 88e09ac2a15d674db9e814755602572be61241ff
> I mostly wonder why it was working fine previously, when it shouldn't.
It looks like when `LCOV_OPTS` is set, during branch coverage, and we end up with something like `lcov -a --rc lcov_branch_coverage=1 baseline_filtered.info`, lcov just reads pass the extra arguments, and still uses the file, i.e:
```bash
touch some_file
lcov -a --rc lcov_branch_coverage=1 some_file
Combining tracefiles.
.. found 1 files to aggregate.
Merging some_fi
...
(https://github.com/bitcoin/bitcoin/pull/28771#pullrequestreview-1734005073)
ACK 88e09ac2a15d674db9e814755602572be61241ff
> I mostly wonder why it was working fine previously, when it shouldn't.
It looks like when `LCOV_OPTS` is set, during branch coverage, and we end up with something like `lcov -a --rc lcov_branch_coverage=1 baseline_filtered.info`, lcov just reads pass the extra arguments, and still uses the file, i.e:
```bash
touch some_file
lcov -a --rc lcov_branch_coverage=1 some_file
Combining tracefiles.
.. found 1 files to aggregate.
Merging some_fi
...
🚀 fanquake merged a pull request: "fuzz: Minor improvements to tx_package_eval target"
(https://github.com/bitcoin/bitcoin/pull/28825)
(https://github.com/bitcoin/bitcoin/pull/28825)
🚀 fanquake merged a pull request: "depends: remove `PYTHONPATH` from config.site"
(https://github.com/bitcoin/bitcoin/pull/28845)
(https://github.com/bitcoin/bitcoin/pull/28845)
🚀 fanquake merged a pull request: "tests: Fix LCOV_OPTS to be in the correct position"
(https://github.com/bitcoin/bitcoin/pull/28771)
(https://github.com/bitcoin/bitcoin/pull/28771)
💬 fanquake commented on pull request "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)":
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1814165459)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1814165459)
Concept ACK
💬 maflcko commented on pull request "tests: Fix LCOV_OPTS to be in the correct position":
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814170922)
Thanks for taking a look. The `--rc` seems wrong as well? Previously it was ignored, but now I wonder why it doesn't lead to errors, because, according to the manpage:
```
Only one of -z, -c, -a, -e, -r, -l and --diff may be specified at a time.
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814170922)
Thanks for taking a look. The `--rc` seems wrong as well? Previously it was ignored, but now I wonder why it doesn't lead to errors, because, according to the manpage:
```
Only one of -z, -c, -a, -e, -r, -l and --diff may be specified at a time.
💬 fanquake commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1814183016)
There are even more since this was last updated, but going to merge this now, and we can follow up again later.
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1814183016)
There are even more since this was last updated, but going to merge this now, and we can follow up again later.
🚀 fanquake merged a pull request: "Fix typos"
(https://github.com/bitcoin/bitcoin/pull/28605)
(https://github.com/bitcoin/bitcoin/pull/28605)
💬 fanquake commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814186095)
Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814186095)
Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
💬 maflcko commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814190598)
> Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
Yeah, it would be a bit cleaner style-wise, if this was merged before C++20. However, it is not a blocker, because the check accepts anything greater than C++17, which should always pass for all supported compilers:
```
static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814190598)
> Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
Yeah, it would be a bit cleaner style-wise, if this was merged before C++20. However, it is not a blocker, because the check accepts anything greater than C++17, which should always pass for all supported compilers:
```
static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
💬 maflcko commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814192749)
I was mostly waiting for a re-reply from @theuni and @hebasto. Though, given that they haven't replied in more than a month now, I presume they are fine with this pull in the current form?
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814192749)
I was mostly waiting for a re-reply from @theuni and @hebasto. Though, given that they haven't replied in more than a month now, I presume they are fine with this pull in the current form?
💬 fanquake commented on pull request "tests: Fix LCOV_OPTS to be in the correct position":
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814195738)
I think `--rc` is different case, not included in that list, so it is also still working here?
> --rc keyword=value
> Override a configuration directive.
> Use this option to specify a keyword=value statement which overrides the corresponding configuration statement in the > lcovrc configuration file. You can specify this option more than once to override multiple configuration statements. See > [lcovrc(5)](https://man.archlinux.org/man/lcovrc.5.en) for a list of available keywords and th
...
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814195738)
I think `--rc` is different case, not included in that list, so it is also still working here?
> --rc keyword=value
> Override a configuration directive.
> Use this option to specify a keyword=value statement which overrides the corresponding configuration statement in the > lcovrc configuration file. You can specify this option more than once to override multiple configuration statements. See > [lcovrc(5)](https://man.archlinux.org/man/lcovrc.5.en) for a list of available keywords and th
...
💬 fanquake commented on pull request "rpc: Remove deprecated -rpcserialversion":
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814199845)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814199845)
Concept ACK.
🤔 BrandonOdiwuor reviewed a pull request: "test: Use test framework utils in functional tests"
(https://github.com/bitcoin/bitcoin/pull/28528#pullrequestreview-1734132495)
There are still some instances of bare assert which could be replaced by the helpers such as in `rpc_psbt.py`, `wallet_disable.py` is this intentional?
(https://github.com/bitcoin/bitcoin/pull/28528#pullrequestreview-1734132495)
There are still some instances of bare assert which could be replaced by the helpers such as in `rpc_psbt.py`, `wallet_disable.py` is this intentional?
💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395555866)
Agree that it should be `true` as a belt-and-suspenders, but we are inside `if (CheckMinimalPush(vch, opcode))` already here, so shouldn't happen?
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395555866)
Agree that it should be `true` as a belt-and-suspenders, but we are inside `if (CheckMinimalPush(vch, opcode))` already here, so shouldn't happen?