💬 jonatack commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2198630776)
I think this makes the logging less useful, as it's less clear if the alert is a warning, an error, or fatal (or possibly if action by the user is required), and this would affect non-developer users who don't change the logging from default settings.

I would suggest simplifying the logging code, rules/restrictions and logging API, and making the latter consistent along with the printed logs. Reducing end-user utility doesn't seem the way to go in reducing the existing complexity.

💬 hebasto commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2198632337)
CI related stuff has to be ported to the CMake staging branch.

💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2198639182)
> I think this makes the logging less useful, as it's less clear if the alert is a warning, an error, or fatal

I don't have a strong opinion about this change, but one thing I like about it is that it reduces the potential for log spam. I think developers will be less likely to use a level called "alert" for minor errors and warnings, and will more appropriately use the debug log level instead.

It is true that this PR gets rid of the ability to distinguish warnings and errors from informat
...

📝 ishaanam opened a pull request: "#27307 follow-up: update mempool conflict tests + docs"
(https://github.com/bitcoin/bitcoin/pull/30365)
#27307 follow-up:
- updates description of `mempoolconflicts` and `walletconflicts` in `gettransaction`
- adds release notes for 27307
- removes unnecessary line from `wallet_conflicts.py`

💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1660239367)
Ditto

💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2198661062)
ACK https://github.com/bitcoin/bitcoin/commit/aba504759caa13338aa574a7839d078b989fbf6d

👍 tdb3 approved a pull request: "optimization: Moved repeated `-printpriority` fetching out of AddToBlock"
(https://github.com/bitcoin/bitcoin/pull/30324#pullrequestreview-2150128035)
re ACK 7759cd87ccf16e1de8f385924daf4d27afd94464

Tested that the arg still worked properly after the change. The message was missing without `-printpriority=1`. The message was present with it.

```
src/bitcoind -debug -regtest -loglevel=trace -printpriority=1
src/bitcoin-cli -regtest createwallet test
src/bitcoin-cli -regtest -generate 101
src/bitcoin-cli -regtest -named sendtoaddress address=bcrt1qcv96frlnhzddst4dnlaa9nvdmvc4darw335wr9 fee_rate=3 amount=2
src/bitcoin-cli -regtest -n
...

💬 ceffikhan commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2198715168)
Hi @pinheadmz ,

Could you please review this pull request when you have a moment? Your feedback would be greatly appreciated!

Thank you!

💬 furszy commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1660304433)
By doing this, you include `common/args.h` in all files that include `node/miner.h`. It is better to add the field to the `BlockAssembler::Options` struct.

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1660309760)
Thanks a lot, it's so much better this way!

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#issuecomment-2198760158)
Thanks a lot for your help @tdb3, appreciate your reviews and verifications!

💬 TheCharlatan commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2198761717)
> I picked libbitcoin_util instead of libbitcoin_common to avoid a future dependency of the Stratum v2 Template Provider (https://github.com/bitcoin/bitcoin/pull/29432) on libbitcoin_common. The latter goal requires additional changes and is mostly just nice to have (the important bit is not depending on libbitcoin_node).

So I guess it should ideally only have the kernel as a dependency? It is not quite clear to me yet how this is going to be achieved. The current mining interface requires kn
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198761817)
@paplorinc was trying to implement your suggestion, but it's much too complex I think.
How do we delete the node that points to the sentinel? If we move the sentinel into the node and reassign sentinel to that node, we would still need to erase that node from the `coinsCache` map. We could extract the node handle from the map and have the `CCoinsViewCache` own that, but this seems like far too much code being changed. I don't think there's that much benefit for removing one pointer per entry. W
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198762814)
@andrewtoth Just high-level brainstorming, and I really don't want to push this PR into something more complex than necessary.

But would it be possible to have a map from outpoints to entries, with a singly-linked list through the entries, such that *(map[key].m_prev) stores the data for key key, rather than map[key] itself?

👍 kristapsk approved a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2150199260)
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198767807)
> But would it be possible to have a map from outpoints to entries, with a singly-linked list through the entries, such that *(map[key].m_prev) stores the data for key key, rather than map[key] itself?

@sipa Sorry I'm not sure I follow. What does *(map[key].m_prev) store?

👍 tdb3 approved a pull request: "optimization: Moved repeated `-printpriority` fetching out of AddToBlock"
(https://github.com/bitcoin/bitcoin/pull/30324#pullrequestreview-2150201952)
re ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6

Re-ran same tests in
https://github.com/bitcoin/bitcoin/pull/30324#pullrequestreview-2150128035

same results.

💬 fjahr commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1660315938)
```suggestion
conflicting transactions can be seen in the `"mempoolconflicts"` field of
```

💬 fjahr commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1660316415)
Linter fails because the import of `COINBASE_MATURITY` is not needed anymore (line 12).

💬 furszy commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1660318367)
nano nit: would call it "DEFAULT_PRINT_FEERATE" as the "modified" word is internal and might change over time.