💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256496328)
I think it would be helpful to explain this in the test.

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256406548)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258

I don't think StartShutdown() can be called twice, but it should be ok if that did happen, like you say.

Checking `index.nHeight > m_stop_at_height` wouldn't be a good way to detect two shutdown calls, though, because that condition can also happen if they the node is restarted with a lower `-stopatheight` value (because the implementation only stops when new blocks are added, and doesn't make any attempt to rewind)

🤔 ryanofsky reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1519716758)
Updated c2bcc339d7b8def58289d3ff01b2a905dc291ed6 -> 09938a41d904c05b4676b064da9baa85e53e3e6f ([`pr/stopafter.2`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.2) -> [`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.2..pr/stopafter.3)) updating -stopatheight documentation to be a little more precise.

I was also going to add a new commit 03dfa85e057192e38d706b8c40a21e70c5aeec91 droppin
...

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1626217041)
@achow101 Done. I think this is quite a bit simpler too, thanks.

@theStack This was a pretty big change so it invalidates your review (but as an upside also removed the code that you commented about).

💬 hebasto commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1626275707)
Concept ACK.

🤔 furszy reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1520042712)
Oh yeah.
The first commit on #27607 extracts the `LoadMempool` call out of the `ImportBlocks` method, second and third one are about renaming the function (and some other fields) and documenting it. Then the rest are improving the indexes initialization process so we can, at the end, erase the indexes global flag.

But the `-stopafterblockimport` changes are new and conceptually looks great in a first glance @TheCharlatan. Happy to review it if you push it into a standalone PR.

Maybe the s
...

📝 ryanofsky opened a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051)
This change just removes some code and gets rid of an unnecessary layer of indirection after #27861

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1626315000)
Note: the branch tests are passing locally. What is failing is the CI compiling it on top of master. Fixing it..

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1626315131)
> Is there some historical reason why we consider IPV6 to be active, even when it's not? I mean sure, attempting connections is pretty cheap, but still?

good question! looks like https://github.com/bitcoin/bitcoin/pull/1021 introduced support of IPV6. there's some PR convo that indicates it started with defaulting off, but review direction lead to it being on.

maybe we could consider a deeper fix that would allow nodes to detect whether IPV6 is actually connectable or not, and enable / di
...

👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1520127552)
Code review ACK 988f3f692acabf0eedfed38e4704fe355f995e72, just suggested StartIndexes simplification and comment changes since last review

💬 zkfrio commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1626337924)
> > Good point and most of the people who tried this with coins involved in any incident regret doing it. Privacy in bitcoin is not as simple as doing some transactions with your own addresses automatically. There are lot of things you need to take care of. One mistake and your privacy is breached, publicly available for the whole world.
>
> What kind of incidents do you mean? Do you have some concrete examples? I don't see how one's privacy would be any worse after sending transactions to on
...

🤔 furszy reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1520172723)
Rebased on master due a silent conflict. Only had to adapt an `AbortNode()` line (which now is under the `fatalError()` alias).

Should be easy to re-check with a `git range-diff 988f3f6...4223d80`.

💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1626374506)
ACK 7f2a985147ef541123c65d5db1c3fc3e533fd4ce

This PR fundamentally doesn't change our implementation of ChaCha20, just its interface so that it matches RFC8439. It otherwise behaves in the same way as previously. The new test vectors match those of the RFC.

💬 luke-jr commented on pull request "Prune locks":
(https://github.com/bitcoin/bitcoin/pull/19463#issuecomment-1626446019)
>Are you still working on this?

Yes, though it seems even rebasing on `branch-25` still results in conflicts, so I'll have to get back to it after Knots v25.

💬 MarcoFalke commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#discussion_r1257082772)
```suggestion
ASSERT_DEBUG_LOG ("failed to validate the -assumeutxo snapshot state");
```

How is this different from just using the helper macro? Also, you can't use the hardcoded program name here. Finally, it may be good to add steps to reproduce/test for reviewers. Otherwise it is unclear what your goal is. Reading the description you are trying to avoid that some error string is output?

💬 MarcoFalke commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1626884095)
Where did you download the binary, what is the checksum/hash of the binary, what is the virus scanner?

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1257088164)
The reason why I left the comment is that `randbytes` is a template function and your wrapper function isn't one, so I fail to see the benefits of the wrapper function. It would be good to mention at least one benefit. Otherwise it seems odd to change code in one direction and then create a follow-up to revert the exact code changes and do some other change.

🤔 MarcoFalke reviewed a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1518337119)
Concept ACK, left some questions.



Concept ACK 6eb33bd0c21b3e075fbab596351cacafdc947472 🗼

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
...

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257118235)
in 6eb33bd0c21b3e075fbab596351cacafdc947472: What is this?

It would be good to split this up in it's own commit, so that it is possible to add a description as to why the changes are needed and beneficial.

Currently it looks like a mistake, because my understanding is that it will just lead to a linker error to call this function somewhere outside of base.cpp

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255459930)
Would it make sense to document that all of these may throw, except for `bool()`?

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707)
Also, it would be good to explain why in the following commits it is fine to then ignore the exceptions and no longer catch them.