💬 sipa commented on pull request "[fuzz] Avoid partial negative result":
(https://github.com/bitcoin/bitcoin/pull/29462#issuecomment-1959050896)
utACK 9dae3b970a7a82e8d9f3f755048d427da78c49da

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1959076888)
Why would they? If the RPCs are not compiled into the binary, how would the test know they exist?

📝 fanquake locked a pull request: "Revert "multiprocess: Add basic type conversion hooks""
(https://github.com/bitcoin/bitcoin/pull/29463)
Reverts bitcoin/bitcoin#28921

💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#issuecomment-1959130540)
re-utACK 36c82cd0e23b4149fb145d2dd271ab1de385c53a

👍 TheCharlatan approved a pull request: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1895491742)
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b

💬 alfonsoromanz commented on issue "RFC: Migration from NSUserNotificationCenter to UNUserNotificationCenter on macOS":
(https://github.com/bitcoin-core/gui/issues/112#issuecomment-1959153389)
I was not familiar with the process of bundling and signing the application, however I ran the commands mentioned (`make deploy`, `codesign -s - ./Bitcoin-Qt.app`, and `open ./Bitcoin-Qt.app`) and the app ran with no issues. Having said that, I don't see an issue in having to run those commands in order to make notifications work when compiling the code

💬 Sjors commented on pull request "Double -dbache maximum to 32GB":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-1959155804)
Fixed broken `feature_config_args.py` test, taking into account the lower limit on 32 bit systems.

💬 Sjors commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#discussion_r1499041071)
Don't forget to drop this.

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