💬 maflcko commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#discussion_r1619018413)
nit: filename

💬 fanquake commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#discussion_r1619022478)
Updated

💬 maflcko commented on pull request "build: remove usage of BOOST_NO_CXX98_FUNCTION_BASE":
(https://github.com/bitcoin/bitcoin/pull/30189#issuecomment-2137599157)
> I think it's unlikely to actually turn much up, given packages are pinned in depends

at least for the sanitizer builds, the compiler is bumped regularly, so it seems good to check those bumps when they happen.

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1619032750)
Using this approach instead of two distinct signatures -- `void push_back(const T&)` and `void push_back(T&&)` -- can lead to some confusion in error messages.

Consider this:
```c++
struct A { int m_a{0}; } a;
struct B { int m_b{0}; } b;

std::vector<A> v;
v.push_back(a);
// ERROR:
// no known conversion for argument 1 from ‘B’ to ‘const std::vector<A>::value_type&’ {aka ‘const A&’}
// no known conversion for argument 1 from ‘B’ to ‘std::vector<A>::value_type&&’ {aka ‘A&&’}
// v.pu
...

📝 m3dwards opened a pull request: "ci: move ASAN job to GitHub Actions from Cirrus CI"
(https://github.com/bitcoin/bitcoin/pull/30193)
Draft PR for moving the ASAN + LSAN + USDT + friends job to github actions from Cirrus.

💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2137638765)
Concept ACK, may review tomorrow if GHA is back online by then.

📝 theuni opened a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194)
Recommended by boost docs:
https://www.boost.org/doc/libs/1_85_0/libs/multi_index/doc/compiler_specifics.html#type_hiding

This significantly reduces the size of the symbol name lengths that end up in the binaries as well as in compiler warnings/errors. Otherwise there should be no functional change.

Example before:

> 0000000000000000 W unsigned long boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_laye
...

💬 hebasto commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2137644075)
Concept ACK.

💬 theuni commented on pull request "refactor: use recommended type hiding on multi_index types":
(https://github.com/bitcoin/bitcoin/pull/30194#issuecomment-2137652409)
Note that I started by limiting `BOOST_MULTI_INDEX_LIMIT_INDEXED_BY_SIZE` and `BOOST_MULTI_INDEX_LIMIT_TAG_SIZE` as also recommended by the docs (which helped), but with the hiding applied with this commit those defines appear to be moot.

👍 fanquake approved a pull request: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049#pullrequestreview-2085710294)
ACK 5deb0b024e14c7c63d405c651d1ca323560a1c21 - given none of this is currently tested/wont compile. Can be revisted in future.

👍 hebasto approved a pull request: "build: remove `--enable-lcov-branch-coverage`"
(https://github.com/bitcoin/bitcoin/pull/30192#pullrequestreview-2085711219)
ACK 462e761672d387c98e729e920a4102feaedc53cb. A similar approach was used in https://github.com/hebasto/bitcoin/pull/191.

💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2137681414)
Question: Do we want to keep this job whole or split parts out (such as just running the USDT tests)?

If we are keeping it whole as it is with ASAN and LSAN etc, I will investigate if we can get the IPV6 tests working. If I can't get them working, I would like to update the python IPV6 check as it's currently passing even though IPV6 isn't working on the runner.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1619089090)
Your code here doesn't use `B` anywhere, so I suspect the comments don't match the code.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2137732304)
Added additional `Assume()`s to prevent `operator++` on iterators past the end:

```diff
--- a/src/util/bitset.h
+++ b/src/util/bitset.h
@@ -101,6 +101,7 @@ class IntBitSet
/** Progress to the next 1 bit (only if != IteratorEnd). */
constexpr Iterator& operator++() noexcept
{
+ Assume(m_val != 0);
m_val &= m_val - I{1U};
if (m_val != 0) m_pos = std::countr_zero(m_val);
return *this;
@@ -272,6 +273,7 @@ clas
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619127144)
I see, then defining `recv_result` as `int64_t` instead of `ssize_t` should be ok on all platforms?

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137755950)
I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4<sup>n</sup> higher than before, and stuck looking to mine a first block at full difficulty.

I previously understood that for the difficulty it just goes back to the _latest_ block that has a non-1 difficulty, and d
...

🤔 TheCharlatan reviewed a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2085822425)
Concept ACK

💬 jsarenik commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137768664)
Tested ACK 86fea43762

👋 marcofleon's pull request is ready for review: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186)

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037)
@vasild I haven't had a chance to thoroughly review the second commit. Making a separate PR could help.