👍 stickies-v approved a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1570159243)
ACK 7721ab9139013d70ef0f058754fabc5aaeda2246

The changes in `blockstorage.cpp` and `blockstorage.h` are strict improvements, increasing visibility in flushing failures. I found the change in `validation.cpp` much more difficult to review, given the wide variety of ways in which `FlushStateToDisk()` is used. I've found that:
- it improves behaviour in some ways. For example, if `FlushBlockFile()` fails and the `WriteBlockIndexDB()` call still succeeds, we can end up in a situation where we ex
...

💬 stickies-v commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288920603)
nit: can be declared closer to where it's used

💬 stickies-v commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1288925171)
nit: long line

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1672973129)
> Anything left to do here, for a required CI-only pull request with review?

I believe this PR is ready to merge unless @fanquake has more comment regarding its description.

However, it requires adjusting "Actions permissions" and "Workflow permissions" in the repo in a similar way as it was done in https://github.com/bitcoin-core/secp256k1/pull/1389.

@achow101 Would you mind taking care of that, please?

💬 fanquake commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1672982779)
Simplifed this further, given we can actually just ask for `llvm-config --cmakedir`.

⚠️ Sjors opened an issue: "Raise maximum -dbcache setting"
(https://github.com/bitcoin/bitcoin/issues/28249)
### Please describe the feature you'd like to see added.

`nMaxDbCache` was set to 16 GB back in 2015: https://github.com/bitcoin/bitcoin/commit/b3ed4236beb7f68e1720ceb3da15e0c3682ef629#diff-d102b6032635ce90158c1e6e614f03b50e4449aa46ce23370da5387a658342fd

The UTXO set has been growing significantly over the past few months so we should probably raise it.

### Is your feature related to a problem, if so please describe it.

Doing IBD without flushing dbcache is faster. That said, I'm not sure
...

💬 Sjors commented on issue "Raise maximum -dbcache setting":
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1672986348)
cc @sipa

💬 theStack commented on pull request "test: locked_wallet, skip default fee estimation":
(https://github.com/bitcoin/bitcoin/pull/28232#discussion_r1289938015)
nit: could use the `get_fee` helper (from test_framework.util) for not having to bother with manual fee-rate unit coversion and fee calculation:
```suggestion
value = inputs[0]["amount"] - get_fee(expected_tx_size, self.min_relay_tx_fee)
```

👍 pinheadmz approved a pull request: "doc: Improve documentation of rpcallowip"
(https://github.com/bitcoin/bitcoin/pull/27480#pullrequestreview-1571626541)
ACK c8e066461b54d745b85411035fcc00a1a4044d76

reviewed code changes, built locally. tested the option with various (in)valid parameters to confirm the docs match the behavior

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK c8e066461b54d745b85411035fcc00a1a4044d76
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmTUwzsACgkQ5+KYS2KJ
yTpDbhAAzRSmfx29YKWZDQ4gY4bCIKF4/PDAwX4DTo3hh7mOpKai7eKx3TPl2RAd
oy42t0
...

⚠️ Vasu-08 opened an issue: "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit""
(https://github.com/bitcoin/bitcoin/issues/28250)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

While executing rpc createmultisig it outputs a addr descriptor wrapped around sh. Addr descriptor is supposed to be a top level descriptor. Please refer to this gist [https://gist.github.com/Vasu-08/57c1ff479baf2e70e7e2195d31575651](url) for further details.

### Expected behaviour

Not sure.

### Steps to reproduce

![image](https://github.com/bitcoin/bitcoin/assets/107955853/a5c3cb23-
...

💬 TheCharlatan commented on pull request "blockstorage: Drop legacy -txindex check":
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1673110008)
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.

I was curious if it might be possible to easily make one of the `CBlockIndex*` in `BlockTreeDB` a `CBlockIndex&`, but the change became a bit sprawling.

👍 TheCharlatan approved a pull request: "doc: use llvm-config for bitcoin-tidy example"
(https://github.com/bitcoin/bitcoin/pull/28245#pullrequestreview-1571755564)
Nice, Re-ACK d82bb90a5b9dc1fd48b10514bdcd8f425aced256

💬 furszy commented on pull request "test: locked_wallet, skip default fee estimation":
(https://github.com/bitcoin/bitcoin/pull/28232#discussion_r1290085370)
sure, pushed. Thanks.
would be nice to have a document somewhere mentioning the existence of these utility functions.

💬 furszy commented on pull request "test: locked_wallet, skip default fee estimation":
(https://github.com/bitcoin/bitcoin/pull/28232#issuecomment-1673173304)
Updated per feedback. Thanks theStack.

Tiny change. Instead of calculating the fee manually, the code now uses
the `get_fee()` util function.

💬 brunoerg commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1290088658)
In https://github.com/bitcoin/bitcoin/pull/28215/commits/c5f6b1db56f67f529377bfb61f58c0a8c17b0127:
```
Note this was never hit because `exists_using_have_coin_in_backend` is
currently never `true` (it's the default implementation of `CCoinsView`.
However this might change if we were to add a target where the backend
is a `CCoinsViewDB`.
```

What do you mean by "add a target"? Because if `exists_using_have_coin_in_backend` is always false, `(!coin_using_access_coin.IsSpent() && exists_u
...

💬 dergoegge commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1290090216)
See #28216

📝 glozow opened a pull request: "validation: avoid mempool eviction mid-package evaluation"
(https://github.com/bitcoin/bitcoin/pull/28251)
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mai
...

💬 glozow commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-1673181801)
cc @instagibbs

🤔 TheCharlatan reviewed a pull request: "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1571859693)
Why not make `NotifyHeaderTip` a private `ChainstateManager` member? The PR title seems to already hint as much.

💬 achow101 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673195541)
I think this was fixed by #28067