💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825267562)
could avoid the duplicate map lookup:
```suggestion
if (auto it = map_txid_ref.find(parent_txid); it != map_txid_ref.end()) {
parent_ref = it->second;
```
(though it probably doesn't really matter that much as `map_txid_ref` won't have more than two entries currently; also in light of https://github.com/bitcoin/bitcoin/pull/30239/commits/92d7ad2b135668cb7f6b0fd32be15fd6dd2f915c#r1816952259)

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825276484)
```suggestion
// Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards
```

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825273227)
potential follow-up nit: could create a helper like `HasDustOutputs` (or `IsDusty`) and use that here and in the `prioritisetransaction` RPC

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825278134)
follow-up idea: could take use of that helper in `mempool_package_rbf.py` (might be a nice "good first issue")

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825284831)
is this change needed? seems to also pass with the dust value

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825279952)
follow-up idea: could put this in a helper for creating an ephemeral package (`dusty_tx` + `sweep_tx`), as that's a repeated pattern in many sub-tests; might make sense to parametrize with tx version, number of dust outputs and optional additional sponsors

💬 kevkevinpal commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2451089685)
Not yet sure on this PR we already have `priority_level` for benchmarks

All the benchmarks being removed seem to be `benchmark::PriorityLevel::HIGH`
not sure if it makes sense to move them down a level?

> This frequently comes up with new contributors and they can't be blamed for thinking that e.g. hex parsing should be made faster if a benchmark test exists.

I understand this is one of the motivations for this change does this happen often for benchmarks that are marked `benchmark::P
...

⚠️ Sandra-Amina-Boss opened an issue: "Cosigner"
(https://github.com/bitcoin/bitcoin/issues/31201)
Zpub6yQaKv5t4M6wxzXVE7GFFLxuA3Zcj6g1baUMr48krvf3VqiZbXzQ6ab6EUT9HwUyCDzU2CJ9u2znpDpbRRQvBfeaF6HHroAUGZryqyZBzuD

✅ achow101 closed an issue: "."
(https://github.com/bitcoin/bitcoin/issues/31201)

:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/31201)

👍 tdb3 approved a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2409329609)
ACK d7fd766feb2f579bdba0e778bacdeb13103e8282

Looks good, thanks.
Corecheck.dev shows the added coverage for mempool.cpp.
Ran a few quick sanity checks.

💬 tdb3 commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1825331169)
Induced failure with:
```diff
- hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list[1]["hex"]]
+ hex_list = [valid_tx_list[0]["hex"], valid_tx_list[1]["hex"]]
```
Also verified that the `not in node.getrawmempool()` asserts also fail, e.g.
```diff
self.log.info("Submitpackage only allows valid hex inputs")
valid_tx_list = self.wallet.create_self_transfer_chain(chain_length=2)
- hex_list = [valid_tx_list[0]["hex"][:-1] + 'X', valid_tx_list
...

👍 theStack approved a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2409334427)
ACK ad5c012157c5f261503022cfa22d7124bfda5765

> On average, this counts about ~100 extra bytes per packet on x86_64:
>
> ```
> 2024-10-27T09:50:12Z [net] 24 bytes -> 112 bytes
> 2024-10-27T10:36:37Z [net] 61 bytes -> 176 bytes
> 2024-10-27T10:36:38Z [net] 1285 bytes -> 1392 bytes
> 2024-10-27T09:50:21Z [net] 43057 bytes -> 43168 bytes
> ```

Seeing very similar numbers here w.r.t. to the extra byte count (tested on OpenBSD 7.6, x86_64):
```
2024-11-01T01:25:18Z msg type: inv, raw s
...

💬 github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2451241236)
> maybe ECC ram would be smarter too... could just be getting hit by cosmic rays or something

Cosmic rays are quite unlikely. But simple error because of failing RAM stick, very likely. You may not even seen this in day to day work. But because bitcoind is writing a lot of data (and everything what it writes go through RAM first), then it may be helping you to expose otherwise unknown hardware problem.

💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451410917)
> I don't have a view on this, but just want to document that this PR does not address the described use case of building for fuzzing without building the fuzz binary ([#31057 (comment)](https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400458885)) which is what motivated making `g_fuzzing` a runtime check.
>
> > ```
> > BUILD_FOR_FUZZING:BOOL=OFF
> > BUILD_FUZZ_BINARY:BOOL=ON
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
...

💬 maflcko commented on issue "CI: Make failure message easier to spot":
(https://github.com/bitcoin/bitcoin/issues/31200#issuecomment-2451427806)
I don't think there is an easy solution to always show the error reason at the tail of the CI log. To expand for each task:

* The lint tests are designed to run all (even if a failure occurs in the first one), so that all failures are collected. However, some of the lint tests print other output, so the failures may not be at the very tail. I've added the `⚠️` emoji to error messages to make them easier to spot, but maybe there are other ways to improve. Example: https://github.com/bitcoin/bi
...

💬 maflcko commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-2451435052)
Yeah, this keeps coming up (ex: https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2450578651), but I don't know how to improve the docs.

Another related idea was to move the CC/CXX/SANITZER/... build options into a cmake preset, which could make it easier to reproduce some CI failures. Ref: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-09-05#1050658 and some more discussion in https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344047118

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

Often there will be no parents (a parent is in the mempool, but been flooded/reconciled already). It would be nice to give a random order in that case.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825649181)
This function could easily return two numbers: how many of these peers are inbound and outbound. And then just pass this pair along to `ShouldFanoutTo`. Should be not that complex code. A cheap cost to preserve the efficiency model.

I might try if you don't get to it earlier.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825659145)
Why having a collision means we should fanout the children? They may get reconciled in their own time.