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

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257121025)
nit in 6eb33bd0c21b3e075fbab596351cacafdc947472: Why are you replacing cassert with assert.h?

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257126777)
Ah nvm. I see it is private and probably needed to access the `m_...` member.

💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1626903191)
It's good enough.

📝 MarcoFalke opened a pull request: "XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052)
Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to P2P or RPC block requests.

Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a ran
...

💬 MarcoFalke commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1257196740)
Looks like this is the only option that is required by the fuzz target, so maybe remove the others for now to hopefully reduce the number of conflicting pulls and remove the dependency on the other pull?

💬 MarcoFalke commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#issuecomment-1627051897)
Can be tested on current master with:

```diff
diff --git a/src/index/base.cpp b/src/index/base.cpp
index cf07cae286..2987eeb97b 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -234,6 +234,7 @@ void BaseIndex::ThreadSync()
__func__, pindex->GetBlockHash().ToString());
return;
}
+ return FatalErrorf("foobar");
}
}

```

Result on master:

```
$ ./src/test/test_bitcoin -t blockfilter
...

📝 TheCharlatan opened a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053)
This has the benefit of moving this StartShutdown call out of the blockstorage file and thus out of the kernel's responsibility. The user can now decide if he wants to start shutdown / interrupt after a block import or not.

Once https://github.com/bitcoin/bitcoin/pull/27607 is merged (splitting up the block import and `LoadMempool` steps), the `return_after_import` bool can be removed.

This also simplifies https://github.com/bitcoin/bitcoin/pull/28048, making it one fewer shutdown call to
...