💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1657659234)
see https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509, I think that FRESH-but-not-DIRTY coins may not exist.

💬 paplorinc commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1657672491)
Rebased https://github.com/bitcoin/bitcoin/pull/30093 after this merge - it became trivial after your changes

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1657685887)
> It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks

I have added these checks to `FinishTransaction`.

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1657687535)
Here the wallet is locked because it runs `CWallet::GetLastBlockHash` and `CWallet::GetLastBlockHeight` which both require `pwallet->cs_wallet` to be locked.

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657710330)
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot.

Between 6 and 8 levels seems to be standard? The most frequent levels might be our current ones, plus Fatal, i.e.

Fatal, Error, Warning, Info, Debug, Trace

https://www.crowdstrike.com/cybersecurity-101/observability/logging-levels/
https://sematext.com/blog/logging-levels/
https://betterstack.com/community/guides/logging/log-levels-explained/
https://stackoverflow.com/questions/2031163/when-to-use-t
...

📝 instagibbs opened a pull request: "policy: Add `OP_TRUE <0x4e73>` as a standard output script"
(https://github.com/bitcoin/bitcoin/pull/30352)
This is a sub-feature taken out of the original proposal for ephemeral anchors https://github.com/bitcoin/bitcoin/pull/30239

This PR would allow protocols or wallets that want to use an anchor pattern
to do CPFP with a *keyless* anchor of any standard satoshi amount 240 or above.

The output is "keyless" because there is no authorization for spending it, and
for a number of use cases authorization is not required or perhaps even desired.
This is a weight usage optimization over `p2sh(OP_
...

👍 stickies-v approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2128509733)
ACK 974934b3c23ce7f34ef46f3dfc13468cd46f4b8a

Reduces dependencies on globals, cleans up the code, and generally makes it more robust through more self-contained code using an RAII-like pattern for `ValidationCache`, not requiring manual initialization of the caches preventing footguns.

I think is good as-is but would prefer addressing various outstanding nits from reviewers as I think some are important enough - will quickly re-ACK myself.

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646478869)
typo nit
```suggestion
//! Pre-initialized hasher to avoid having to recreate it for every hash calculation.
```

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646473144)
This seems incorrect, we clamp the number of elements, not the number of bytes. I'd prefer not documenting the clamping behaviour here, given how generous the limit is.

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657671482)
nit: \*ducks\* it seems like now would be the perfect time to rename this to `SignatureCache`?

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657442012)
reviewer note: orthogonal to this move-only change, but this has all kinds of under- and overflow issues. Since it's a debug issue, i suppose it's not the end of the world though

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657682792)
nit: any reason this isn't initialized where it's used?

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657457261)
I didn't realize the mutex has the same practical effect when I mentioned it it in my original [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)) - but I too would still prefer to be more explicit here. If for some reason that's not a good idea, can we at least add a comment to the mutex to indicate that it also serves this purpose in case it is updated in the future?

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646343915)
nit: `std::optional` no longer needed as an include

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195528186)
> Also, potential improvements like #28945 will make up for it.

It seems https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953 was abandoned since

💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195529824)
> an extra 13 characters of noise each time (20 if you don't do something to avoid having to type `BCLog::Level::Debug`)

Extra complexity and code and an inconsistent API to avoid "extra characters" (that moreover many developers likely just cut and paste as needed) seems a poor (and borderline silly?) trade-off unless forced on us for technical reasons. Far better to have the code and API as simple and consistent as possible.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195533792)
> > A non-pruned IBD with full dbcache to tip ended up using 12% more memory, but it was also 2.7% faster somehow.
>
> I'm not sure if I understand this, is this still the case? Why would a node use more memory after this pull? Shouldn't the 2 added pointers be included in the dbcache accounting, resulting in more frequent flushes due to the cache filling up faster, but not in an increase in memory when the cache is full?

This is no longer the case. When I did the initial benchmarks the fu
...

💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195538575)
Two years ago, it seemed we had a road map on severity-level logging based on a PR discussion. It turned out not to be the case, and not sufficiently specified, with subsequent proposals to swerve here or take another road there.

Not sure, but perhaps it would be best to gather a larger consensus on which log levels to adopt, and with which definitions, and an overall API, rather than everyone opening competing proposals. But I haven't been following all of these proposals closely.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195540246)
> > Also, potential improvements like #28945 will make up for it.
>
> It seems [#28945 (comment)](https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953) was abandoned since

In my recent benchmarks it was again slightly faster on this branch for default dbcache and no pruning. So I think this PR is essentially the same.

📝 furszy opened a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353)
Fix https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1657628378 inconsistency.

Block 'fundrawtransaction' from automatically picking up inputs when testing the funding of a transaction using only pre-selected inputs.