💬 achow101 commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#issuecomment-1614950818)
ACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df

Checked that the changes here resolve the remaining review comments on #27214

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248094492)
I think I also like the `src/node` option most, but it's ok for me to keep in init for now and move that later.

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1247996232)
Although `GetFirstStoredBlock()` is currently used only for rpc and indexes, this added assert makes me a bit uneasy. While we can check that `GetFirstStoredBlock()` is only called with the tip, how sure can we that the tip has `BLOCK_HAVE_DATA`?
I could think of several edge cases where we don't have data for the tip, and managed to trigger the assert in some of these situations:
1. (tested): With AssumeUTXO, right after loading a chainstate from disk / before getting any additional blocks fr
...

🚀 achow101 merged a pull request: "addrman: select addresses by network follow-up"
(https://github.com/bitcoin/bitcoin/pull/27745)

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1248139828)
I understand your worries on coupling the validation within the constructor (separation of/ different concerns, error handling, flexibility, etc) but since those validations/ "early checks" also would impact into the state of the object itself it would make sense to me to put them together, consistently and cleaner. Happy to make the change if @vasild also agree with it.

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1615002228)
I've encapsulated the 96-bit nonce into a `using Nonce96 = std::pair<uint32_t, uint64_t>`, which makes the interfaces a bit cleaner.

🤔 Xekyo reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1507669515)
Renamed the functions on the Chain interface to better express their utility. Added a convenience function for accessing bump fees to SelectionResult.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248149182)
I added a function `GetSummedBumpFees()`, but I have yet to find a neat way how I can drop the discount. I was experimenting with having both a function `GetSummedBumpFees()` and a function `GetCombinedBumpFee()` on SelectionResult, but it made it more complicated because I now had to get the Chain interface and target feerate into `SelectionResult`. I am tending to leaving it as it is now, unless I come up with a more elegant way to transition.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248150186)
Still open

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248149983)
Added `GetSummedBumpFees()`

💬 sipa commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1615016422)
ACK 4f4d039a98370a91e3cd5977352a9a4b260aa06b

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248159158)
> So I think if we now require that "The caller must ensure that upper_block has block data available." we should adjust the existing callers to do that.

Is there something you would like to see specifically, like adding asserts at the call site? I thought most of the call sites were just passing the chain tip, so it's hard to see how this could cause a problem unless the tip was pruned, in which case the current assert failure would be seem pretty appropriate

📝 fanquake opened a pull request: "gui: drop macOS ForceActivation workaround"
(https://github.com/bitcoin-core/gui/pull/744)
Discussion in https://github.com/bitcoin/bitcoin/pull/16720 seems to point to this being no-longer needed after qt 5.6+.
We now require 5.11.x+.

💬 sipa commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#issuecomment-1615036465)
utACK fac6af16f4a254458b8cb3522317422b40362f8d

The two commits feel quite unrelated here, though, but both seem fine to me.

👋 fanquake's pull request is ready for review: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992)

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248166855)
I don't think an assert failure is appropiate if having a pruned tip is a valid state that bitcoin core can be in, which seems to be the case, at least in the examples above. For example, it should be possible to call `getblockchaininfo` directly after loading a snapshot from disk, without triggering an assert.

🚀 fanquake merged a pull request: "test: add python implementation of Elligator swift"
(https://github.com/bitcoin/bitcoin/pull/24005)

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248168420)
Based on the `AssumeUTXO` line. It seems to be expected to not have block data available right after importing the chainstate. And that shouldn't crash the software if the user calls `pruneblockchain` or `getblockchaininfo`.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248182230)
I see, so maybe current code is buggy and returns bad rpc results then? I guess simplest thing to do is just document GetFirstStoredBlock's odd behavior in this case and keep it unchanged, rather than returning nullptr which could segfault, or adding an assertion.

The new GetFirstStoredBlock function should be able to straightforwardly return false in this case, though.

💬 TheCharlatan commented on pull request "ci: re-enable gui tests for s390x":
(https://github.com/bitcoin/bitcoin/pull/28014#issuecomment-1615104483)
Post-merge ACK 9be4565c2db6d7a420d032d5c41843d473ed32d1