🤔 jonatack reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1519649211)
Tested Approach ACK

💬 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_r1256494108)
805692f

<details><summary>Various suggestions</summary><p>

```diff
diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index 6c97eed9d86..87c6082b3e0 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -14,6 +14,7 @@ from test_framework.util import check_node_connections, assert_equal, p2p_port

INBOUND_CONNECTIONS = 5
BLOCK_RELAY_CONNECTIONS = 2
+ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nub
...

💬 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_r1256380724)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 nit, can be simplified

<details><summary>diff</summary><p>

```diff
def check_addrv2(ip, net):
addr = CAddress()
- addr.net = net
- addr.ip = ip
+ addr.net, addr.ip = net, ip
ser = addr.serialize_v2()
actual = CAddress()
actual.deserialize_v2(BytesIO(ser))
- self.assertEqual(actual.ip, ip)
-
...

💬 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_r1256363333)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 naming, this would be more readable in case of a test failure (also, remove 2 extra LF at EOF)


<details><summary>diff</summary><p>

```diff
class TestFrameworkScript(unittest.TestCase):
- def test_addrv2encodedecode(self):
+ def test_addrv2_encode_decode(self):
def check_addrv2(ip, net):
@@ -1905,5 +1905,3 @@ class TestFrameworkScript(unittest.TestCase):
check_addrv2("fc32:17e
...

💬 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?