💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657322116)
Move this into `ValidationCache`? It seems unused anywhere else.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657354399)
Same comment here, would be nice to pass this in. Perhaps one nonce to rule them all? I don't think there'd be any loss of security.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657370573)
Similar to @stickies-v's comment in a previous commit, since we're going to be passing around references and pointers to this, it'd be a good idea to make it non-copyable to avoid accidental copies.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657337310)
Might be useful to pass the nonce in rather than generating it here to make the order of rng/ValidationCache instantiation explicit.

Presumably it'd also make for easier testing with a deterministic seed.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657394662)
Oh, I guess the mutex takes care of that for us. Still, wouldn't hurt to be explicit.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850893)
Thanks, fixed

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657289988)
Made all four tests output messages that indicate what went wrong in the success case.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1629892844)
Removed the static keyword and the clear as suggested.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657397165)
Amended all the functions to directly create OutputGroups instead.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850804)
I have removed `nInput` from the function parameters of `MakeCoin(…)`.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622850394)
I was able to remove all except for

```cpp
#include <consensus/amount.h>
#include <wallet/coinselection.h>
#include <wallet/test/wallet_test_fixture.h>

#include <boost/test/unit_test.hpp>
```

I’m not familiar with IOWY and could not turn up anything with a websearch, what is that?

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622920206)
Thanks, fixed

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1622920679)
Thanks, fixed

👍 1alexbc1 approved a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2145858736)
Amadeevantv yimelaja

💬 ariard commented on issue "Brainstorm: Transaction issuer-selected policy limits":
(https://github.com/bitcoin/bitcoin/issues/29454#issuecomment-2195109286)
> The problem with policy limits is they're all based on asking miners to ignore transactions that are potentially profitable. This is similar in spirit to opt-in RBF. And as we've seen, the supermajority of hash power eventually decided to ignore the opt-in flag and just do RBF all the time. If push came to shove, it wouldn't be surprising to run into the same class of problem with policy limits.

> Much better to focus on solutions like replace-by-fee-rate that rely on better aligning miner
...

💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657420121)
> I’m not familiar with IOWY and could not turn up anything with a websearch, what is that?

Upps, the acronym is IWYU from include-what-you-use. I wrote it fast as IOWY "include only what you use".
Should stop using acronyms..

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2195114617)
You can also drop the `BerkeleyDatabaseSanityCheck` related suppression in `contrib/devtools/check-devs.sh`

💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2195118674)
Alright, i can't reproduce the non-determinism he observed.

🤔 furszy reviewed a pull request: "Wallet: Add `max_tx_weight` to transaction funding options (take 2)"
(https://github.com/bitcoin/bitcoin/pull/29523#pullrequestreview-2145904445)
utACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6

💬 furszy commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1657436891)
In 734076c6de178:

nano nit: shorter
```suggestion
if (*coin_selection_params.m_max_tx_weight < minimum_tx_weight || *coin_selection_params.m_max_tx_weight > MAX_STANDARD_TX_WEIGHT) {
```