💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236188801)
> Yes - the wallet would consider any UTXOs included in these transactions as spent, and any UTXOs they create would be spendable. Of course there would need to be consideration in a wallet UI to avoid creating a situation where non-existent UTXOs are spent in a broadcasted transaction (which would simply fail on broadcast as it does now).

I wonder if the feature helps users in that case. If a set of transactions isn't in "the mempool" (the one the wallet is connected to) right now, it will p
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682577501)
Now that we have this, could you please cover it with tests as well (where we're testing that the linked list is constructed correctly)?
Or would it make more sense to just call `SanityCheck()`since it's already checking the linked list's integrity?

<img src="https://github.com/user-attachments/assets/782fc1a0-8d78-4ad2-a963-8e77337a8a39">

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682588039)
This is only called in the [fuzz tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coinscache_sim.cpp), right?

Is it just bad luck that it wasn't triggered by CI or could the fuzz tests be incorrect or just not yet triggered at this stage?
<img width='400px' src="https://github.com/user-attachments/assets/196ed1a1-7c2c-4181-b0e6-d19eaf789e4a">

Could we use it in `coins_tests` or `coinscachepair_tests` as well?
And if it's a test-only method, maybe document, that it's h
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682615656)
Any reason not returning directly like in `Begin()`?
```suggestion
inline CoinsCachePair* Next(CoinsCachePair& current) noexcept { return current.second.Next(); }
```

💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682629622)
Fuzz coverage isn't covered by corecheck. It is produced daily at https://maflcko.github.io/b-c-cov/fuzz.coverage/src/coins.cpp.gcov.html (for master).

It can also be produced (manually triggered) for this pull request, if you think it would be useful.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682638514)
Thanks for the info.
I assumed that some of the randomized tests contributed to the coverage here. Isn't that why we have `Lost baseline coverage` for PRs? Or did I misunderstand the reason for unrelated code not being covered anymore?

💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682651787)
> I assumed that some of the randomized tests contributed to the coverage here.

The section where your screenshot is from is "Uncovered new code". The screenshot means that the code wasn't covered previously and isn't covered now either. There should be no randomness.

💬 glozow commented on issue "Change estimate_mode default to "ECONOMICAL" in these RPC calls":
(https://github.com/bitcoin/bitcoin/issues/30009#issuecomment-2236230361)
> This code suggests it does, but I haven't tested:

@Sjors that code is for wallet getting an estimate when it's making a BIP125-signaling tx.

People on this thread may want to review #30275

📝 hebasto opened a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477)
The `{C,CXX,CPP,LD}FLAGS` are build type-agnostic. Therefore, if any of them is specified, it should be assigned to a non-type-specific variable.

This PR is split from https://github.com/bitcoin/bitcoin/pull/30454. It is required because CMake distinguishes [build type-specific variables](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG.html).

No behaviour change for Autotools:
- on the master branch @ efbf4e71ce8e3cd49ccdfb5e55e14fa4b338453c
```
$ make -C depends pr
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682658330)
yes, of course.
I assumed that the existence of `Lost baseline coverage` means that there was some randomness in the tests or infrastructure or production code or threading or something (i.e. covered before, but not covered now for a reason that's unrelated to this change):
<img width="400" alt="image" src="https://github.com/user-attachments/assets/b319ec1f-cde8-4a83-9e17-8e5949af0cf5">

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2236249062)
My Guix build:
```
x86_64
0d9925c3463e882ead8b87010ef9ce477107cdb3d1da54c9e50a70ed7c128463 guix-build-218e69c705d7/output/aarch64-linux-gnu/SHA256SUMS.part
0fea765c772ee00ec37d4efde6296e173a8a765792534a731e64b253effd9242 guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu-debug.tar.gz
4a2c51df14f82f5947f12ec28274d6b65f42e7e03f766d1a52937b7fa4075bbf guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu.tar.gz
f061bd6ff
...

💬 fanquake commented on pull request "depends: Amend handling flags environment variables":
(https://github.com/bitcoin/bitcoin/pull/30477#issuecomment-2236252027)
It would be good to show an example of what this actually fixes.

> No behaviour change for Autotools:

Libevent in depends is built with CMake, so if this is fixing something for CMake, shouldn't this have some effect in master (for the libevent build done with CMake) regardless of if Autotools is used later for the main configure?

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1682669806)
I dropped the first commit and use default help text as @ajtowns suggested.

---
Yes I think it will be beneficial to improve the documentation
I will open up https://github.com/bitcoin/bitcoin/commit/2ee3d774815f5e1840cb37d2a92007f19c217e4c in a separate pull.

Thanks _all_ for your reviews.

💬 hebasto commented on pull request "depends: Amend handling flags environment variables":
(https://github.com/bitcoin/bitcoin/pull/30477#issuecomment-2236267842)
> Libevent in depends is built with CMake, so if this is fixing something for CMake, shouldn't this have some effect in master (for the libevent build done with CMake) regardless of if Autotools is used later for the main configure?

In depends, the `$(package)_{c,cxx,ld,cpp}flags` variable are used for both build systems, Autotools and CMake.

It is required for a new `toolchain.cmake` files generated in https://github.com/bitcoin/bitcoin/pull/30454: https://github.com/bitcoin/bitcoin/blob/
...

💬 VojtechMyslivec commented on issue "gettransaction details does not include send to myself balance changes for imported addresses":
(https://github.com/bitcoin/bitcoin/issues/28555#issuecomment-2236269937)
If you send a transaction from one of your address to another one in the same wallet, sum of `amount`s would be zero and in details, you would see one _receiving_ and one _sending_ balance change.

This is one of my example transaction:

```
{
"amount": 0.00000000,
"fee": -0.00004200,
...
"txid": "...",
...
"details": [
{
"address": "...y",
"category": "send",
"amount": -0.00012000,
"fee": -0.00004200,
...
},
{
"addr
...

🤔 glozow reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2185560501)
ACK 41a2545046bce315af697a3c6baf6e3fb2e824c2

Checked that default `estimatesmartfee` result is using economical.

💬 maflcko commented on pull request "test, refactor: Fix MSVC warning C4101 "unreferenced local variable"":
(https://github.com/bitcoin/bitcoin/pull/30464#issuecomment-2236275618)
ACK 44f08786f435ed4284d39dc604c2a5fcbde9e602

💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682689533)
Yes, that is unrelated and deterministic tests should be added.

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1682690671)
No it doesn't. I have changed the type to `uint8_t`

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1682693031)
In https://github.com/bitcoin/bitcoin/pull/30243/commits/0b30b690e7067240b05ece17fdc7ad44549368a3 I now use `TryParseHex` and just check for the presence of a response