💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256329428)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

This skips trying to flush the undo file if flushing the block file fails. It think it would be safer and more conservative to keep previous behavior of flushing both files in sequence.

You can still return false if either flush call fails, but I don't see a benefit in changing behavior here and potentially making code less robust, when it's not necessary just to add a return code.

I think
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256284494)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

Let me know if I understand this correctly: The new `return false` avoids writing block data to a new block file if the _previous_ block file is full and cannot be synced and trimmed. This seems to be safe because this `FindBlockPos` function only has one caller, `SaveBlockToDisk`, which only has two callers, `Chainstate::AcceptBlock` and `Chainstate::LoadGenesisBlock` which both seem to handle t
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256350394)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

This change seems fragile to me, and it's unclear what the benefits are. I think it would be great to add a log print here, but if this this code is going to be changed to return false, I think that should happen in a separate followup commit, not combined with other changes here, and have a clear explanation and justification.

IIUC, this change introduces an inconsistency. For all block data,
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256378064)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

This line appears to implement the major changing in behavior which is avoiding the "write from
`WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet."

I wonder what this failure looks like in practice, though. Could we write a test for it? I think it would at least make sense to have a log print that explains the problem that's happening.

There are also a *l
...

📝 furszy opened a pull request: "test: make assumeUTXO test capture the expected fatal error"
(https://github.com/bitcoin/bitcoin/pull/28050)
The test is exercising the error, so it can capture it before the
test framework displays it on the console as an unforeseen
fatal error.

It is odd to observe a fatal error after executing the complete
test suite.

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

I'm glad you changed your mind on returning things from the notifications, putting the infrastructure in place to allow the code to now bubble a `-stopatblock` interrupt is a clear advantage to me.

I was about to open a similar PR, albeit with a different approach for handling `-stopafterblockimport` ([patch](https://github.com/TheCharlatan/bitcoin/commit/1107a234838d7a3999dbf236c8a7e4e00d48f289)). Instead of creating a new notification for it, I thought slightly refa
...

💬 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_r1256352616)
f6bd653

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

```diff
@@ -76,11 +76,11 @@ TEST_FRAMEWORK_MODULES = [
"blocktools",
"ellswift",
"key",
+ "messages",
"muhash",
"ripemd160",
"script",
"segwit_addr",
- "messages"
]
```
</p></details>

(Also, project convention is to leave a comma after the last element, to minimize the diff in future changes)

🤔 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
...