💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825044678)
I added a longer explanation to this test. Luckily 18 is not generally the limeit, it’s just the limit in this artificially crafted case where all of our UTXOs have very similar same amounts and we have to pick eight of them to meet the target. Please let me know if it makes sense now! :)

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825000480)
Yeah, as explained, the intention is to rewrite all coinselection tests with transactions that could be encountered in the wild rather than the artificial zero-fee transactions the old tests are using.

🤔 murchandamus reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2408805041)
Thanks @furszy, @nymius, @jasonribble, and @vicariousdrama for the review!
I updated the commit message names as suggested, adopted all of your proposed changes as indicated, and added more explanation to the test covering the attempt limit.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825047813)
Thanks, I’ll make a note to update the benchmark as well.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2450644019)
> Also, do you think is worth to add these conventions to the `CONTRIBUTIONS.md` file?

I don’t know! It would be a good thing to ask in the IRC channel or at the weekly meeting. :)

💬 ryanofsky commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450648747)
Maybe a flexible way to support this would be to add wait options to the `createNewBlock()` method so instead of creating a new block template right away, it could optionally wait for conditions to be reached. Those conditions could be set by the client or determined by the node.

Something like:

<details><summary>diff</summary>
<p>

```diff
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -90,9 +90,13 @@ public:
*
* @param[in] script_pub_key the coinbase o
...

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825058258)
ok so I think I was a bit wrong, these values were being used and would "blind" us potentially from catching the "BUG! Mempool ancestors or descendants were underestimated" error.

💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2450652369)
> An easy first fix would be to downgrade InitError to InitWarning, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)

Do we also need to explicitly remote the entry from settings.json as well, to make sure it doesn't appear at every start after that. Or does this happen automatically in re-saving it somehow?

⚠️ m3dwards opened an issue: "CI: Improve documentation around replicating CI locally"
(https://github.com/bitcoin/bitcoin/issues/31199)
I've received some feedback that replicating CI jobs locally isn't straightforward. Perhaps the documentation can be improved in this area?

CC @maflcko

⚠️ m3dwards opened an issue: "CI: Make failure message easier to spot"
(https://github.com/bitcoin/bitcoin/issues/31200)
Sometimes during a CI failure the actual error message can be buried in a wall of log output and not immediately clear. Ideally it should be obvious why a CI job has failed.

CC @maflcko

💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450797714)
@ryanofsky that approach makes sense to me. In that case perhaps `waitFeesChanged()` won't be needed as its own method #31003? That has a nice side-effect in the current implementation of letting us return the last generated block template (I hope eventually we can get the next-block-fees without generating templates at all).

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825202486)
Yes, as far as i can tell it's an artifact of switching between protocols. I should remove it. In fact i think `g_mapport_enabled` can be removed as well as we can just use `g_mapport_interrupt` to stop it.

📝 l0rinc converted_to_draft a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144)
The recently merged [XOR obfuscation](https://github.com/bitcoin/bitcoin/pull/28207) is a non-trivial part of serialization, encountered during IBD and `reindex`.

In this PR, I've added a new test (comparing randomized inputs against a trivial implementation), updated the benchmark to validate more realistic scenarios (collected every call of `util::Xor` for the first 400k blocks and modelled the benchmark to have the same distribution of data) and optimized the Xor function to do the work in
...

👍 tdb3 approved a pull request: "init: warn, don't error, when '-upnp' is set"
(https://github.com/bitcoin/bitcoin/pull/31198#pullrequestreview-2409213226)
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0

Sanity check below.

On master (f07a533dfcb172321972e5afb3b38a4bd24edb87), prevents start:


PR branch (a1b3ccae4be82297fd20f5be15a03eeb477507d0), doesn't:

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825257255)
Done.

💬 davidgumberg commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2450980584)
Tested ACK

This solves https://github.com/bitcoin/bitcoin/issues/31178

`./build/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11 -min-time=30000`

|branch| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|------------|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|master (f0
...

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825258139)
Done for both `current` and `enabled`.

💬 kevkevinpal commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2451022302)
Concept ACK [a1b3cca](https://github.com/bitcoin/bitcoin/pull/31198/commits/a1b3ccae4be82297fd20f5be15a03eeb477507d0)

makes sense to give a warning instead of giving an error which is bad ux for a user who has been using `-upnp`

🤔 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)
```