⚠️ pinheadmz opened an issue: "on OSX, bitcoind chooses different data directory than Bitcoin-Qt"
(https://github.com/bitcoin/bitcoin/issues/27119)
bitcoind uses `~/Library/Application Support/Bitcoin` as base data directory path but Bitcoin-Qt uses `~/.bitcoin`

I actually much prefer the latter since it is more UNIX-like and I assume users running the daemon from the command line probably do as well. It might make sense if this behavior was swapped and the GUI chose the user-friendly library/support path, but my preference would be using `~/.bitcoin` on OSX all the time.

The issue most likely comes down to this logic in system.cpp:

...

💬 sipa commented on pull request "Optimizations & simplifications following #25717":
(https://github.com/bitcoin/bitcoin/pull/25968#discussion_r1110031910)
How could it be faster? It's just a function that runs the provided lambda in a loop...?

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1434867163)
Since last comment:

- Rebased on-top of the latest version of #27021.
- Addressed that `CalculateTotalBumpFee` now returns an std::optional<CAmount> because we automatically fail calculation for (too) large clusters of unconfirmed transactions
- Waiting for #27021 before opening for review

💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1434870979)
review-beg-pinging @LarryRuane @josibake @stickies-v who have looked at `MiniMiner` previously

👍 hebasto approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)
re-ACK 52f4d567d69425dfd514489079db80483024a80d

👍 hebasto approved a pull request: "refactor: remove windows-only compat.h usage in random"
(https://github.com/bitcoin/bitcoin/pull/26814)
re-ACK 621cfb77227b5a240d66547947f73130f0c51f44, rebased only since my [recent](https://github.com/bitcoin/bitcoin/pull/26814#pullrequestreview-1237312144) review. Verified with:
```
git range-diff master 4e7bed297a6dd1ae0da52a7eb99659849f18f259 621cfb77227b5a240d66547947f73130f0c51f44
```

👍 ryanofsky approved a pull request: "validation: Improve error handling when VerifyDB dosn't finish successfully"
(https://github.com/bitcoin/bitcoin/pull/25574)
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups

💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110031541)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (0c7785bb2540b69564104767d38342704230cbc2)

Might be worth adding a comment to explain why you would set the option. Maybe:

```c++
//! Setting require_full_verification to true will require all checks at
//! check_level (below) to succeed for loading to succeed. Setting it to
//! false will skip checks if cache is not big enough to run them, so may be
//! helpful for running with a small cache
...

💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110050930)
In commit "validation: return VerifyDBResult::INTERRUPTED if verification was interrupted" (d6f781f1cfcbc2c2ad5ee289a0642ed00386d013)

It would be less surprising to have `VerifyDBResult::INTERRUPTED` return `ChainstateLoadStatus::INTERRUPTED` than `ChainstateLoadStatus::SUCCESS`. Another potentially good side effect would be that it would avoid the caller printing a misleading load time if the load didn't really succeed:

https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e
...

💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110037026)
In commit "validation: Change return value of VerifyDB to enum type" (6360b5302d2675788de5c4a28ea77d823f6d809e)

Commit message says this does not change behavior, but this is changing log prints slightly replacing `VerifyDB():` prefix with `Verification error:`. This is probably a good change, but could be clarified in the commit message.

💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109999734)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107700526

Nice, it's good that returning in one place makes it easier to see what the precedence is, or change what's returned in the future.

💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110019015)
re: https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1109110569

Thanks for implementing the suggestion. This comment was not about `bitcoind`, but about code that uses libbitcoinkernel such as:

https://github.com/bitcoin/bitcoin/blob/fe1b3256888bd0e70d0c9655f565e139ec87b606/src/bitcoin-chainstate.cpp#L93-L96

Code like this has it's own options and defaults, and is not aware of `checkblocks` or `checklevel`. It is just calling and using the `LoadChainstate` function directly. Fr
...

💬 willcl-ark commented on issue "on OSX, bitcoind chooses different data directory than Bitcoin-Qt":
(https://github.com/bitcoin/bitcoin/issues/27119#issuecomment-1434919095)
FWIW I could not replicate this on my 2017 Intel MBP on master. Choosing "Use Default Datadir" on QT setup screen saw it use "Library/Application Support/Bitcoin" as the datadir.

⚠️ mistercx opened an issue: "Hidden fee (about 15% of sum) while send"
(https://github.com/bitcoin/bitcoin/issues/27120)
<!-- Describe the issue -->
Hello,

My wallet is [3KxxEaECid1H1h5Ya2XnjsxLx8fK6TToQj](https://www.blockchain.com/explorer/addresses/btc/3KxxEaECid1H1h5Ya2XnjsxLx8fK6TToQj) - 18 transactions total.

I sent 0.1BTC, but in blockchain I found 2 transactions: my own (0.1BTC) + unknown fee (0.01999241BTC) - 20% of sum!
TXID: [70a951a496a809597b79753be7b4f05a7b13ece1d09d8d0ae643b0190ace1fba](https://www.blockchain.com/explorer/transactions/btc/70a951a496a809597b79753be7b4f05a7b13ece1d09d8d0ae643b
...

💬 hebasto commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434933175)
> Changed the approach here, from using libevents own hardening option

The previous 974e44c0a0e692e1e11e7c067699db94f55ce464 commit, being combined with the current 778e34e8625cc83d0e5a93493c71f01712bef81d one, should work for older compilers as well, no?

💬 hebasto commented on issue "Hidden fee (about 15% of sum) while send":
(https://github.com/bitcoin/bitcoin/issues/27120#issuecomment-1434951175)
Are those "hidden payments" your own [change](https://bitcoin.stackexchange.com/search?q=change)?

💬 willcl-ark commented on issue "Hidden fee (about 15% of sum) while send":
(https://github.com/bitcoin/bitcoin/issues/27120#issuecomment-1434956574)
Yes, Bitcoin Core will use a new address to send the change to. If you add up the amounts on the other addresses, you will reach the Bitcoin Core balance:

1.35997247 + 0.0199924 + 0.00997949 = 1.38994436

💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434971844)
> should work for older compilers as well, no?

I changed the approach because I don't want us to use the gcc-hardening option.

💬 llazzaro commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1434978191)
Yes, with custom exception the review is really hard (it took me some hours to discover those exceptions).
If you agree I can switch back to Exception, the change is more simple and less error prone.

💬 hebasto commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#discussion_r1110114839)
> ... can be omitted if \<condition\>

It creates additional burden for reviewing if \<condition\> will change in the future, doesn't it?