🤔 theStack reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2328771065)
Concept ACK

Mostly looked and reasoned about the core commit 92d7ad2b135668cb7f6b0fd32be15fd6dd2f915c so far. From what I understand the "only one dust output" limit would not be strictly necessary and is far less important than the other two rules (must be 0-fee, child must spend all dust from the parent) in order to disisincentivize creating dust UTXOs, or am I missing something?

What I left below are largely nits and follow-up ideas, feel free to ignore. Still want to look deeper into u
...

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825270642)
nit: calling the RPC via the `.rpc` member shouldn't be needed? (here and below)
```suggestion
dust_txid = self.nodes[0].sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0)
```

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825269811)
```suggestion
the dust is both created and spent simultaneously.
```

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825272593)
whitespace nit
```suggestion
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
```

💬 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
...