💬 m3dwards commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1819804495)
@hebasto thank you for the great suggestion. I have added a test for it in the testing guide.

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819817301)
The latest state is https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742. I could tackle it if @murchandamus is busy with something else.

💬 brunoerg commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819833040)
So, if the problem is SFFO, can we fix fuzz by doing:
```diff
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index 4caf96b18d..1e040fb5e2 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -116,12 +116,14 @@ FUZZ_TARGET(coinselection)
}

// Run coinselection algorithms
- auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT);
-
...

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819842590)
I don't think hiding issues under the carpet is a good path to follow.

🤔 pablomartin4btc reviewed a pull request: "coins: make sure PoolAllocator uses the correct alignment"
(https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1740761357)
Concept ACK

👍 TheCharlatan approved a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1740752299)
ACK 7268aaf36bdcc6b466fd16bbf544782571e680bb

💬 TheCharlatan commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399789113)
Nit: Since you're already making `hash` `const`, maybe make `txHeight` and `feeRate` `const` too?

📝 ryanofsky opened a pull request: "multiprocess: Add basic type conversion hoois"
(https://github.com/bitcoin/bitcoin/pull/28921)
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.

The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.

---

This PR is part of the [process separation project](https://gith
...

💬 brunoerg commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819906879)
> I don't think that hide issues under the carpet is a good path to follow.

Correct me if I'm wrong. In https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742, isn't the solution that we will adopt to disable BnB when SFFO is enabled?

💬 kevkevinpal commented on pull request "bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC":
(https://github.com/bitcoin/bitcoin/pull/28554#issuecomment-1819926688)
reACK [9ac114e](https://github.com/bitcoin/bitcoin/pull/28554/commits/9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658)

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819948545)
> > I don't think that hide issues under the carpet is a good path to follow.
>
> Correct me if I'm wrong. In [#28395 (review)](https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742), isn't the solution that we will adopt to disable BnB when SFFO is enabled? I was wondering whether we would end up touching the fuzz doing that.

Yes. We need to tackle both aspects in the same PR (wallet and fuzz test). Skipping BnB only in the test side would hide the unexpected behavior
...

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1399855267)
One possible problem I encountered when running the `p2p_invalid_messages.py` `test_resource_exhaustion` subtest with v2 that but could affect many tests intermittently, also this spot:

I think that there could be a race between the python main thread of functional tests and the p2p thread:
For example, this command 1) builds messages, 2) encrypts them and 3) sends them, all within the main thread.
If during step 2) the bitcoind node would decide to send out a ping, the p2p thread will also
...

👍 BrandonOdiwuor approved a pull request: "script, assumeutxo: Enhance validations in utxo_snapshot.sh"
(https://github.com/bitcoin/bitcoin/pull/28852#pullrequestreview-1741060818)
tested ACk 11b7269d83a56f919f9dddb7f7c72a96d428627f

1. Successfully tested error handling added and chainstate can be automatically recovered if an error occurs e.g when the snapshot file already exists or user interruption by `ctrl + c`
2. Successfully tested the option to enable or disable network activity

💬 maflcko commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1820392508)
lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 47e5c9994c087d18
...

💬 maflcko commented on pull request "lint: Report all lint errors instead of early exit":
(https://github.com/bitcoin/bitcoin/pull/28862#issuecomment-1820396967)
> tested on my own fork and saw multiple lint errors

Yeah, I guess linters should enforce their rules globally, instead of only on the diff. Otherwise, one gets different results, depending on how the diff is calculated.

Though, this is unrelated to this pull request and can be improved in the future, if there is need and interest.

💬 maflcko commented on pull request "multiprocess: Add basic type conversion hooks":
(https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400158381)
```suggestion
DataStream stream{};
```

The values are ignored either way, so it seems better to remove them. `CDataStream` will go away anyway, soon.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1820479487)
Concept ACK

You could also add a `oldest_timestamp` (or `birth`) field to `getwalletinfo` which would contain the birth date.

🤔 Sjors reviewed a pull request: "wallet: birth time update during tx scanning"
(https://github.com/bitcoin/bitcoin/pull/28920#pullrequestreview-1741418506)
I checked that the test fails if you drop everything but 134960c3828abe658d565af52e3c6c5af03019f2.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400220704)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: alternative name: `MaybeUpdateBirthTime( )`

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400226671)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: this seems simpler:

```cpp
TryUpdateBirthTime(spkm_man->GetTimeFirstKey());
m_spk_managers[id] = std::move(spkm_man);
```

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400224214)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: I'm a bit puzzled why this function ever accepted an argument that it didn't use.