👍 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?

💬 achow101 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1435007548)
ACK 52f4d567d69425dfd514489079db80483024a80d

🚀 achow101 merged a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1435034303)
For anyone wanting to review this PR and would like some help with basic mempool concepts, I made a video: https://youtu.be/sQ05azzTp9o -- it mentions 26152 but I think would be helpful for reviewers here as well.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1110150658)
This is good feedback, and in the general case you're right. In the specific case of the deferred checks being introduced here, they don't include any of the computationally expensive stuff that motivates parallelizing checks in the case of CScriptCheck - i.e. we're not doing any cryptographic ops or signature checking here; OP_VAULT deferred checks consist of summing and comparing integers.

So I'm not necessarily averse to doing the deferred-check parallelization in this changeset, but (i) I
...

💬 LarryRuane commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1435036691)
For anyone wanting to review this PR and would like some help with basic mempool concepts, I made a video: https://youtu.be/sQ05azzTp9o.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1110154588)
I looked at using `std::variant` but the end use in `ValidateDeferredChecks` winds up being much hairier. I've changed the specific checks from `std::unique_ptr` to `std::optional` for simplicity's sake. If we eventually decide that the memory implications of this approach are too heavyweight, we could do something with std::variant - but I think they're comparable, and given that we'll never have more than a few thousand deferred checks in flight at any given time, I think it's probably okay to
...