💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657406844)
Should be snake_case.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657411032)
Remove spaces after comments describing params.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657408296)
`nInput` is unused, let’s remove it.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657411595)
Outdated comment, it’s an OutputGroup not coin now.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657415795)
```suggestion
const auto result = SelectCoinsBnB(utxo_pool, selection_target, /*cost_of_change=*/default_cs_params.m_cost_of_change, /*max_weight=*/MAX_STANDARD_TX_WEIGHT);
```

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657413647)
Should be a reference.

```suggestion
static void TestBnBSuccess(std::string test_title, std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const std::vector<CAmount>& expected_input_amounts, const CoinSelectionParams& cs_params = default_cs_params)
```

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657425209)
remove spaces

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657416339)
Remove spaces after param comments

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657423140)
```suggestion
CoinSelectionParams high_feerate_params = init_default_params();
high_feerate_params.m_effective_feerate = CFeeRate{25'000};
std::vector<OutputGroup> high_feerate_pool; // 25 sat/vB (greater than long_term_feerate of 10 sat/vB)
```

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657487890)
Space

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1657504370)
I added a commit, that add more information about the modes.

On master b27afb7fb774809c9c075d56e44657e0b607a00c
This is the help output
```
2. estimate_mode (string, optional, default="conservative") The fee estimate mode.
Whether to return a more conservative estimate which also satisfies
a longer history. A conservative estimate potentially returns a
higher feerate and is more likely to be sufficient for the desired

...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2195282495)
I've addressed the outstanding comments, I believe, including:
* Rename `allow_deterministic` to `always_use_real_rng` (with inverted meaning).
* Dropped `GetRand()` (but not `GetRand(nRange)`), inlining it instead.
* Removed `GetOSRand` from the public random.h interface, and moved it into the anonymous namespace.
* Fixed/improved a few comments
* Dropped most of the specializations in `randbits<BITS>`.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657537328)
Done.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539130)
Moved `GetOSRand` to the anonymous namespace (meaning it can stay in its relative location in random.cpp, reducing the diff size), and dropped it from random.h.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539323)
Done, in a new commit.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539584)
Fixed.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2195289826)
Alright, should hopefully be ready to review.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543406)
Yeah, I don't expect this to matter for `FastRandomContext`, where the production of randomness is expensive enough that it doesn't matter. But for `InsecureRandomContext`, I want to make sure the compiler creates a suitably specialized instance of `randbits` for use in `randbool` for example. While not done in this PR, the original reason for starting to work on this PR is because I want a fast `InsecureRandomContext` for use in #30286.

That said, you're right that all the different manual s
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543513)
Fixed.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543755)
Nice, done.