💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862708127)
New TODO?

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1863509899)
Incomplete refactor:
```suggestion
if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel);
if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel);
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862754511)
`CoinEntry` could be `constexpr`-friendly from the very start instead of gaining that ability towards the end, meaning `SPENT_DIRTY` & co could be `constexpr` from the start, reducing noise.

Could also place `struct CoinEntry` below the old `DIRTY`/`FRESH` constants in order to decrease size of diffs.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862760164)
(nit: Introduced 2 commits before they're used, and `<string>` is only used transiently, where constants could be using `auto` (implicit `char[]`) from the start).

💬 Sjors commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2507864614)
TIL about this, despite having authored #30356.

I think `-blockmaxweight` should refer to the actual worst case block, i.e. assuming the pool uses all 4000 WU. So its default should be equal to `MAX_BLOCK_WEIGHT`.

We do need to make sure that if a miner has been setting `-blockmaxweight=4000000` manually, nothing breaks.

An additional argument for having `-blockmaxweight` not adjust for the reservation, is that in Stratum v2 each connected Template Provider (i.e. a pool or an individual
...

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2507869514)
I'm in favor of fixing this bug, but differently, see https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2507864614

👍 TheCharlatan approved a pull request: "cmake, qt: Use absolute paths for includes in MOC-generated files"
(https://github.com/bitcoin/bitcoin/pull/31361#pullrequestreview-2469994097)
ACK 6f4128e3a838d03f46d397c15bc5333287e14863

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1863578230)
yes, see the mentioned derisked PR: https://github.com/bitcoin/bitcoin/pull/30673/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R152-R154

💬 andrewtoth commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1863578380)
I don't think this change has any affect, aside from requiring `std::move` at the callsite.

In the method body we are only moving out of the individual elements, leaving a hollow element in its place. However, the actual vector `vChecks` is not moved so this would not have any change over using a regular lvalue reference.

This would be a benefit if somewhere in here we took ownership of `vChecks`, like
````
if (queue.empty()) {
queue = std::move(vChecks);
}
```

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1863578642)
If I touch again, I'll fix the curlies :)

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2507906763)
Something like this should do:

```diff
diff --git a/src/init.cpp b/src/init.cpp
index f79ebe881d..881a254efc 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -54,6 +54,7 @@
#include <node/mempool_persist_args.h>
#include <node/miner.h>
#include <node/peerman_args.h>
+#include <node/types.h> // for BlockCreateOptions
#include <policy/feerate.h>
#include <policy/fees.h>
#include <policy/fees_args.h>
@@ -645,7 +646,7 @@ void SetupServerArgs(ArgsManager& argsman, bool
...

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2507914836)
> Surprised that CCoinsCacheEntry::m_next and m_prev were not nulled out before this PR

This was brought up but I didn't change it https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687007975. It does seem cleaner nulling them though.

💬 willcl-ark commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2508001697)
I synced mempool's fork of electrs using bitcoin core v28.0 and there were no issues with unconstrained memory. (also wow; I did not notice in the readme this requires > 1.3TB of disk space to sync)

Memory usage as I measured peaked at about 1.9GB for `bitcoind`, compared with 6.1GB for `electrs`. This machine has 64GB RAM so total utilization was low. `electrs` did crash and require me to increase the max file descriptor count (with `ulimit -n`) which I wasn't expecting, but after that every
...

💬 maflcko commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2508010777)
> Memory usage as I measured peaked at about 1.9GB for `bitcoind`, compared with 6.1GB for `electrs`.

I'd say that explains the crash happening with 4 GB of RAM, but not with 8GB of RAM (albeit that seems a bit lucky).

Not sure what to do here. I don't think there is anything that can be done to catch an (expected) OOM?

💬 maflcko commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1863675186)
> I don't think this change has any affect, aside from requiring `std::move` at the callsite.

Yes, and the benefits of requiring `std::move` are:

* Self-documenting code, to mark an object as used.
* use-after-move static analysis to catch violations, where a depleted object is re-used.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1863681310)
I tried to implement this, but (compile-time) annotations are either useless, because they are not applied globally, or they are too verbose for this, because they need to be applied globally/virally.

I think doing the check at runtime with a bool is probably good enough. Though that requires modifying the header file in some way or another.

An alternative could be to forbid the direct use of the depleting member functions (with a linter) and add a new helper which eats a `FuzzedDataProvid
...

💬 theStack commented on pull request "util: Drop boost posix_time in ParseISO8601DateTime":
(https://github.com/bitcoin/bitcoin/pull/31391#issuecomment-2508060097)
Concept ACK

💬 instagibbs commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r1863733057)
can expand this test case to be a little more interesting with a grandparent as well:
```
diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index 00b94d1a71..8aa5b43a53 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -62,18 +62,18 @@ class PackageRelayTest(BitcoinTestFramework):
"-datacarriersize=100000",
"-maxmempool=5",
]]
self.support
...

💬 willcl-ark commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2508119954)
Oh yes, it could do. I had interpreted OP to mean bitcoin core had 4GB (or 8GB) of RAM to itself, but if both of those were trying to share that amount, it seems like it might not work. I note that over in #31039 it was reported that:

> The machine I use has 4GB and there is nothing other than Bitcoin Core on it.

I got suspicious of my own earlier results as I've seen bitcoin core using more memory than this previously, so re-ran an electrs sync to block 600,000 only this time. `bitcoind`
...

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2508149376)
This PR incidentally "fixes" the IPC bug discovered in https://github.com/bitcoin/bitcoin/pull/31318.