💬 maflcko commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-1958997514)
rebased

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1959006624)
review ACK 16baf8651d17ac1e0c7d7fd3526c19ad018ceb9c

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1498907558)
@murchandamus Indeed, my suggestion was wrong. The overall result never underflows, just the temporary subexpression.

💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1498934743)
If I'm reading this right, not specifying a failure value will lead to it being default constructed, so this will have a value of `ChainstateLoadError::FAILURE`. Could omitting a failure value be a compile-time error if the failure type is non-void?

💬 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