🤔 murchandamus reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2225335817)
ReACK 3b6ac646ffea930ae7d8c5d93acaba3587cc9dcd

💬 murchandamus commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1707091091)
In d6f67e97b7ec0b5fdc39e5ad52e0c03dd0e03fd1 wallet: Keep track of transaction outputs owned by the wallet:

It would have helped me if this function had some documentation. Also, "Wallet transaction transaction outputs" feels a bit redundant, and the name `RefreshWalletTxTXOs(…)` is extremely similar to `RefreshAllTXOs()` that gets introduced in the later commit "wallet: Recalculate the wallet's txos after any imports". If you have to touch this again, perhaps consider calling this function `C
...

💬 murchandamus commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1707520564)
In 9d3762913ba02d1f6cd3b42889c2c3b31f03e90c "wallet: Recalculate the wallet's txos after any imports":

Perhaps add a documentation comment here along the lines of:

```suggestion
/** Inspects every wallet transaction and caches all outputs that belong to our wallet. */
void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
```

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276397742)
@zawy12: I think this gets a bit off-topic here, perhaps it would be better to post about your timewarp scenario to the mailing list or Delving Bitcoin.

📝 andrewtoth opened a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611)
Since #28233, periodically writing the chainstate to disk every 24 hours does not clear the dbcache. Since #28280, periodically writing the chainstate to disk is proportional only to the amount of dirty entries in the cache. Due to these changes, it is no longer beneficial to only write the chainstate to disk every 24 hours. The periodic flush interval was necessary because every write of the chainstate would clear the dbcache. Now, we can get rid of the periodic flush interval and simply write
...

✅ andrewtoth closed a pull request: "validation: sync chainstate to disk after syncing to tip"
(https://github.com/bitcoin/bitcoin/pull/15218)

💬 andrewtoth commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#issuecomment-2276409584)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/30611.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2228236248)
Updated 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 -> c538ec69f266b51c893a374a4bb82796ede3d7cb ([`pr/mine-types.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.3) -> [`pr/mine-types.4`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.3..pr/mine-types.4)) with review suggestions.

Thanks for the review and thoughtful questions. I wrote detailed answers in line but I will reuse the text & diagram in
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709774619)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708937001

> I'm having a hard time reading this TODO. Can you try and reformulate it?

I'm removed the TODO since it's a low-priority efficiency improvement. Basically, there are 2 ways to call `CustomReadField` and there are 2 ways to deserialize certain objects, and the code is not currently choosing the most efficient way to deserialize based on the way it is called.

To explain further, the `CustomReadField` function is re
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1710015402)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708943992

> Typo nit: s/canproto/capnproto/ (here and elsewhere).

Thanks, fixed

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709823817)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040

> Not directly related to this PR, but did these type hook methods get renamed since the docs were written? https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/design/multiprocess.md#type-hooks-in-srcipccapnp-typesh

Good catch, updated the docs to mention these. The custom message hooks have actually been around a while but I moved them from bitcoin code to libmultiprocess code recently in https://github.com/chaincod
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709911230)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709048212

> I'm a bit confused by this file. It is only included by `mining.capnp`, but in turn includes the auto-generated header from `mining.capnp` here. What is the purpose of this? Couldn't the capnp file just directly include `interfaces/mining.h`? At least doing so compiles for me.

I simplified this as suggested. I had just copied these mining interface files from one of the other interfaces (probably node or chain) and
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709972901)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709096542

> I think it would be nice to test some more invariants here:

Thanks, added

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709816237)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643

> Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).

It's a good point but I think modifying serialize.h is outside the scope of this PR, so I will leave it alone here.

I'm not sure a `g_` prefix is ideal since this is a constant, so all caps `DESERIALIZE` would probably communicate its purpose more accurately and be more in spirit of the the developer notes.

I th
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2276410668)
An [announcement](https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo) about the migration to the CMake build system was made on the mailing list. Therefore, more comments from fellow reviewers and testers are expected.

To let us maintain focus on making progress, the following statement has been add to the PR description:

We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented se
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710058792)
It is a TODO comment. I'll update it if/when this file will be touched for other changes.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710065883)
Source files were separated in two groups: fixtures and tests themselves. Both groups are sorted alphabetically.

That seemed convenient to me while I was working on this script.

💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710012397)
I was also wondering, checked the actual execution for both paths (with & without `[[likely]]`), and if the benchmarks are accurate, AppleClang 15 doesn't seem to make any change in this case (yet?).

<details><summary>FeeFracEvaluate bench</summary>

> cmake --build build && ./build/src/bench/bench_bitcoin -filter='FeeFracEvaluate.*' --min-time=10000

// tried a few different ways of testing it, none of them showed any difference

```C++
#include <bench/bench.h>
#include <util/feefrac
...

💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710070948)
Checked the same with a simple logging to make sure the reproducer is representative:
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
index d91689c3b3..0198edbbbe 100644
--- a/src/arith_uint256.h
+++ b/src/arith_uint256.h
@@ -12,6 +12,7 @@
#include <limits>
#include <stdexcept>
#include <string>
+#include <iostream>

class uint256;

@@ -212,7 +213,10 @@ public:
friend inline base_uint operator*(const base_uint& a, uint32_t b) { return base_uint(a) *= b; }
...

💬 andrewtoth commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2276431786)
Concept ACK

Indeed, the `FlushStateMode::ALWAYS` variant is now confusing. It could mean we want to always write the chainstate to disk, or we want to always erase the dbcache. Splitting it up into the two cases makes sense.

💬 achow101 commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2276461122)
ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d