💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1959252129)
Added milestone, since this test can catch bugs, and it was reviewed.

💬 maflcko commented on pull request "ci: avoid running git diff after patching":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1959258909)
lgtm ACK 84388c942cb035fed546eda360ae6b40c6cfac09

I can't review this, since it is written in bash, but given that CI passed and something is printed in the log, I guess this is fine :man_shrugging:

```
+ export HOST=i686-pc-linux-gnu
+ HOST=i686-pc-linux-gnu
+ tee /dev/fd/63
++ patch -p1
--- a/src/leveldb/db/db_impl.cc
+++ b/src/leveldb/db/db_impl.cc
@@ -1028,9 +1028,6 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) {
stats.bytes_read += compact->compaction->i
...

💬 maflcko commented on pull request "ci: avoid running git diff after patching":
(https://github.com/bitcoin/bitcoin/pull/29461#issuecomment-1959259885)
Added milestone, since this is a bugfix

💬 maflcko commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1959265584)
Also, CI still fails

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1895634501)
Updated 20556598140030237861d21a61f646252002ddff -> cdf7bb17563b92e48b0576a0975c168619c5aa34 ([`pr/bresult2.46`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.46) -> [`pr/bresult2.47`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.47), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.46..pr/bresult2.47)) adding a check to requiring scalar failure values to be specified explicitly (https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1498934743)

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1499122381)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1499084153

> But we could add a check for specifically for enums.

Latest push adds a slightly more general check that doesn't allow any scalar failure value (int, float, enum, or pointer) to be default constructed. Could adjust this condition or make it an option if default-constructing these types of values seems useful in the future.

This check is arguably too heavy handed because it is enforcing that the `result.GetFailure
...

💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499130243)
Split into separate commit and expanded on the comment.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499033207)
nit: In the first commit. I tested this locally, and the suppressions were not needed. The cirrus link you provide says:

```
node1 stderr rpc/blockchain.cpp:1658:42: runtime error: unsigned integer overflow: 0 - 200 cannot be represented in type 'unsigned int'
```

Looking at the `0`, it seems more likely that this was introduced in a later commit.

Also, Cirrus will clear the log in 90 days, so it may be better to not link to it.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499026543)
nit in the first commit: Why claim they are fake when one can test which ones are fake?


```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 49cd2d8ec1..1f0d4bb309 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -309,6 +309,17 @@ class AssumeutxoTest(BitcoinTestFramework):
}
self.wait_until(lambda: n1.getindexinfo() == completed_idx_state)

+ self.log.info
...

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499040796)
second commit: LogPrintf is deprecated. Also, could use `STR_INTERNAL_BUG` to reduce boilerplate.

💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499130527)
Split the refactoring into a separate commit.

💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499130753)
Done

💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1499213263)
Nice, the compile-time check can be tested with:
```diff
diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
index 261e1f0b9b..053b45fceb 100644
--- a/src/test/result_tests.cpp
+++ b/src/test/result_tests.cpp
@@ -28,0 +29,10 @@ BOOST_AUTO_TEST_SUITE(result_tests)
+enum class DummyError {
+ FAILURE_1,
+ FAILURE_2,
+ FAILURE_3,
+};
+
+util::Result<void, DummyError> VoidDummyError() {
+ return util::Error{Untranslated("void fail.")};
+}
+
@@ -203,0 +214,3
...

👍 TheCharlatan approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1895838904)
Re-ACK cdf7bb17563b92e48b0576a0975c168619c5aa34

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1959446160)
I was still waiting for someone to suggest a new value. But it seems fine to bump to 64 GB in order to support `verifychain 4 0`, so added that.

@maflcko do you prefer to drop the limit? We should not quietly change a user setting, so imo an invalid value should explicitly fail, rather than silently fail.

> Also, CI still fails

Indeed. I copied the `is_64bits = sys.maxsize > 2**32` check from `netutil.py`, but it seems the check doesn't actually work. So there might be a bug in `netutil
...

📝 Sjors converted_to_draft a pull request: "Double -dbache maximum to 32GB"
(https://github.com/bitcoin/bitcoin/pull/28358)
Due to recent UTXO set growth, the current maximum value for `-dbcache` of 16GB is just months away from being insufficient.

This PR increases the maximum to 32GB. It also adds a warning that it's up to users to check that they have enough RAM.

Finally, we make startup abort if the value provided is too high, rather than quietly rounding it down. This doubling seems like a safe time to do that, so that users who picked a reasonable, but too previously high, round number like 20,000 won't e
...

💬 maflcko commented on pull request "Double -dbache maximum to 64GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1959462383)
i686 is 32 bit and python on that CI machine is compiled to 64 bit

💬 gdiscord commented on issue "Guix build script incorrectly reporting there is no Mac SDK":
(https://github.com/bitcoin/bitcoin/issues/29449#issuecomment-1959473980)
Been trying to work out where this error is coming from. Openssl is supposed to have been completely removed, right?

Pointers on how to resolve will be appreciated.

checking whether the linker accepts -Wl,--exclude-libs,ALL... yes
checking for openssl/crypto.h... no
configure: error: libcrypto headers missing

👍 brunoerg approved a pull request: "[fuzz] Avoid partial negative result"
(https://github.com/bitcoin/bitcoin/pull/29462#pullrequestreview-1895898566)
crACK 9dae3b970a7a82e8d9f3f755048d427da78c49da

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1499270150)
Leftover from previous changes, thanks, I'll remove it.