💬 m3dwards commented on issue "Intermittent CI failure "fee too high" in wallet_send.py":
(https://github.com/bitcoin/bitcoin/issues/25164#issuecomment-1959193449)
Seen this on my local windows machine:

```shell
2024-02-21T21:14:35.394000Z TestFramework.node0 (DEBUG): TestNode.generate() dispatches `generate` call to `generatetoaddress`
2024-02-21T21:14:36.537000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\test_framework.py", line 131, in main
self.run_test()
File "C:\Users\Max\source\bitcoin/test/functional/wallet_send.py", line 577, in run_te
...

💬 maflcko commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1499073687)
> See https://en.wikipedia.org/wiki/Principle_of_least_astonishment

If I imagined myself running into this error. I'd be indeed astonished and ask myself why the dbcache is capped at all. Again, if there is a reason for this change, it should be explained. It can't hurt to put the reason in the error string.

💬 maflcko commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1959217904)
I still don't understand the state of the current pull request. This leaves the currently hard-failing use case un-addressed (https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1698663976) and adds more hard-failing behavior for no reason (https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1310404672)?

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

> Could omitting a failure value be a compile-time error if the failure type is non-void?

Nice catch. That's an interesting idea but it seems extreme because it would make it difficult to call a failure value's default constructor, and impossible to call it the class is not copyable or movable.

I think in general if you don't want a failure value to be default-constructed you should pass a failure type that doesn't
...

💬 maflcko commented on issue "Intermittent CI failure "fee too high" in wallet_send.py":
(https://github.com/bitcoin/bitcoin/issues/25164#issuecomment-1959245635)
@m3dwards Can you run the test with `--tracerpc` to get more relevant values?

💬 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
...