💬 ryanofsky commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1624092185)
If anyone else wants to review: this PR has two acks and is a pure refactoring and I think could be merged if there is another reviewer.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1624093362)
@willcl-ark thanks for the review!

> Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped... I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it... This appears to be because all networks are set as [Reachable by default ](https://github.com/bitcoin/bitcoin/blob/c325f0fbae2d6472a78be733d499783f8b69d6b8/src/net.h#L1
...

🤔 ryanofsky reviewed a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1517040699)
Noticed in IRC libbitcoinkernel updates:

> \<achow101> it looks like the current pr is #27866, although #27861 has also been getting review

IMO, this PR could be an improvement, but right now seems like a draft change. I think the main thing that's missing is a clear description of how it changes behavior, and why the behavior change is safe.

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1254749159)
re: https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227107197

> introduced granularly for each fatal error call site here: [TheCharlatan#13](https://github.com/TheCharlatan/bitcoin/pull/13).

Thanks, it's good to see nodiscard being used there, and introducing more meaningful error codes.

It's possible my suggestion and your PRs have slightly different goals. Maybe your goal is to enhance libbitcoinkernel applications ability to handle and report errors, and my goal is to impr
...

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254760855)
Did about 40M executions, and no issues! Gonna calculate the Waste score below here as discussed.

💬 achow101 commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1624141412)
light ACK 6eb33bd0c21b3e075fbab596351cacafdc947472

I'm not particularly experienced with this area of the code, so I can't really comment on approach taken here. However mechanically, the code looks find and correct to me and seems to achieve the stated objectives.

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1624158995)
re-ACK 20b49460b35268db59f7efcb02736b0e31191a74

📝 furszy opened a pull request: "wallet: address book migration bug fixes"
(https://github.com/bitcoin/bitcoin/pull/28038)
Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it.

Bug 1: Non-Cloning of External 'Send' Records
The external 'send' records were not being correctly cloned to all wallets.

Bug 2: Persistence of Empty Labels
As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookD
...

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1254840912)
> Maybe your goal is to enhance libbitcoinkernel applications ability to handle and report errors, and my goal is to improve clarify of validation code and find to potential bugs in it.

I think that hits it on the nail. In any case I updated the PR description with much of the body of the commit message to make it clearer what I am trying to achieve here and why I'd consider it an improvement.

📝 theuni opened a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039)
Only `#include` upstream bdb headers from our cpp files.

It's generally good practice to avoid including 3rd party deps in headers as otherwise they tend to sneak into new compilation units. IMO this makes for a nice cleanup.

There's a good bit of code movement here, but each commit is small and _should_ be obviously correct.

Note: in the future, the buildsystem can add the bdb include path for `bdb.cpp` and `salvage.cpp` only, rather than all wallet sources.

📝 theuni opened a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040)
Only `#include` upstream sqlite headers from our cpp files.

Like #28039 but simpler :)

💬 Xekyo commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1624254517)
Concept ACK

📝 1proprogrammerchant opened a pull request: "Merge pull request #1 from bitcoin/master"
(https://github.com/bitcoin/bitcoin/pull/28041)
pull_1

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests t
...

✅ hebasto closed a pull request: "Merge pull request #1 from bitcoin/master"
(https://github.com/bitcoin/bitcoin/pull/28041)

📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/28041)
pull_1

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests t
...

💬 mzumsande commented on issue "EstimateMedianVal returns higher fee for higher confTarget":
(https://github.com/bitcoin/bitcoin/issues/20725#issuecomment-1624279055)
> @ismaelsadeeq Can you try with the commit specified in OP as well? If it fails, can you try bisecting?

Maybe not necessary:
There are newer crashes reported in #23165 (which seems to be the same problem). The seed `7288975409214300634` from https://api.cirrus-ci.com/v1/task/6110951150190592/logs/ci.log leads to a crash on current master (the test passes with #21161 rebased on master).

💬 achow101 commented on pull request "wallet: address book migration bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28038#issuecomment-1624292395)
ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9

Nice catch!

🚀 ryanofsky merged a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861)

💬 achow101 commented on pull request "wallet: sqlite: don't include sqlite files from our headers":
(https://github.com/bitcoin/bitcoin/pull/28040#issuecomment-1624315734)
ACK bea9fc2600635020fd28ec7a6613c92a6f349a86

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254925898)
Sure, applied. Thanks!

💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1624321895)
@santochibtc "Feerate histogram" here is a bytes vs fee diagram, not a #tx vs fee diagram.