👍 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
(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
(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);
}
```
(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 :)
(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
...
(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.
(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
...
(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?
(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.
(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
...
(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
(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
...
(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`
...
(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.
(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.
📝 hebasto opened a pull request: "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports"
(https://github.com/bitcoin/bitcoin/pull/31394)
The `gcov`-based code coverage does not work with Clang (please refer to https://github.com/bitcoin/bitcoin/issues/31047 for more details).
This PR employs the LLVM's [Source-based Code Coverage](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html).
Here are some examples of usage:
```
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_FOR_COVERAGE=ON
cmake --build build --target total_coverage
firefox build/test_bitcoin.coverage/index.html
```
or
```
...
(https://github.com/bitcoin/bitcoin/pull/31394)
The `gcov`-based code coverage does not work with Clang (please refer to https://github.com/bitcoin/bitcoin/issues/31047 for more details).
This PR employs the LLVM's [Source-based Code Coverage](https://clang.llvm.org/docs/SourceBasedCodeCoverage.html).
Here are some examples of usage:
```
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_FOR_COVERAGE=ON
cmake --build build --target total_coverage
firefox build/test_bitcoin.coverage/index.html
```
or
```
...
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2508176320)
In light of https://github.com/Sjors/bitcoin/issues/71#issuecomment-2508157740 I think `bitcoin-mine` should call _all_ interface methods. But this can wait for a followup, maybe by myself.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2508176320)
In light of https://github.com/Sjors/bitcoin/issues/71#issuecomment-2508157740 I think `bitcoin-mine` should call _all_ interface methods. But this can wait for a followup, maybe by myself.
✅ arik-so closed a pull request: "Disable RBF Rule 2"
(https://github.com/bitcoin/bitcoin/pull/30964)
(https://github.com/bitcoin/bitcoin/pull/30964)
💬 arik-so commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2508493608)
Thank you so much for your plentiful and insightful responses! I will close this PR and am very much looking forward to cluster mempool obviating it entirely.
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2508493608)
Thank you so much for your plentiful and insightful responses! I will close this PR and am very much looking forward to cluster mempool obviating it entirely.
👍 BrandonOdiwuor approved a pull request: "cmake, qt: Use absolute paths for includes in MOC-generated files"
(https://github.com/bitcoin/bitcoin/pull/31361#pullrequestreview-2470476711)
ACK 6f4128e3a838d03f46d397c15bc5333287e14863
(https://github.com/bitcoin/bitcoin/pull/31361#pullrequestreview-2470476711)
ACK 6f4128e3a838d03f46d397c15bc5333287e14863
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1863916545)
Great that you made time to look into this!
> 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 `FuzzedDataProvider&&` and returns the bytes in it. This way, the existing use-after-move checks can be used.
So you mean disallowing direct calls to `ConsumeRemainingBytes*`-family of methods only, and provide a helper that does only that part instead after swallowing up `FuzzedDataProvider&&` so it should no lon
...
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1863916545)
Great that you made time to look into this!
> 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 `FuzzedDataProvider&&` and returns the bytes in it. This way, the existing use-after-move checks can be used.
So you mean disallowing direct calls to `ConsumeRemainingBytes*`-family of methods only, and provide a helper that does only that part instead after swallowing up `FuzzedDataProvider&&` so it should no lon
...