💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388285003)
Since we are only in test land here, it's probably fine to also use seed-determined randomness for garbage as well (i.e. `random.randrange`)? (I've used `os.urandom` in the past for a PR and got critical feedback about that: https://github.com/bitcoin/bitcoin/pull/25625#discussion_r924540431, which I agree with)
```suggestion
self.sent_garbage = random.randrange(garbage_len)
```

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388279451)
nit: could deduplicate code by using a helper method like `generate_garbage` (or even `generate_keypair_and_garbage`) that is used both in `initiate_v2_handshake` and `respond_v2_handshake`? Could also put the magic number in a `MAX_GARBAGE_LEN` constant (that's the naming we use in the C++ codebase), though that's one less than the number passed to randrange here.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388353134)
nit: could import this constant from test_framework.crypto

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388344334)
nit:
```suggestion
if is_decoy: # since decoy messages are ignored by the recipient - no need to wait for response
```
(or "don't get processed" or something like that?)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388337105)
readability nit:
```suggestion
kwargs['services'] = (kwargs['services']|NODE_P2P_V2) if 'services' in kwargs else (P2P_SERVICES|NODE_P2P_V2)
```

💬 maflcko commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1804300886)
lgtm ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5

I did not check the performance difference against 7e644308805229ab64455c01a22531756644fe69

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1804327607)
I've addressed feedback from @andrewtoth - thank you!

💬 maflcko commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388449752)
Thx, done

💬 maflcko commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388449815)
Thx, done

💬 maflcko commented on pull request "multiprocess compatibility updates":
(https://github.com/bitcoin/bitcoin/pull/28721#issuecomment-1804394759)
re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆

<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=
trusted comment: re-ACK 3b70f7b6156cb110c47
...

💬 fjahr commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1804437895)
> Would it make sense to only check in the asmap.dat file into the repository, and then have the build system convert it to a .h at compile time (similar to the approach used in the `src/test/data` directory)? That would mean there is also a directly-manipulable file in the repository (that can be used with `asmap-tool` etc).

Yeah, that would work as well. I have drafted that approach here: https://github.com/fjahr/bitcoin/commit/0f9109fc49b8add9c057dcacd537250d4539ae57 Details can change of
...

💬 fjahr commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1804443164)
> Why is the option of loading it from a file even in dev builds, not considered?

It is considered. Only loading from a file is the only option for dev builds if we go the other route mentioned in the description: "The alternative approach is not having the data in the source code but only adding it during the build phase of a release." But this also comes with the aforementioned downsides.

💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1804450152)
No, that would be likely to cause problems in various workflows. It is entirely reasonable that -checkblockindex is set in a bitcoin.conf file, just as it is done by default in the RPC tests. In that case restarting the daemon after loading a snapshot but before validation of the historical blockchain is complete will cause it to be in an unstartable state, even though there is nothing wrong with the block index.-checkblockindex is useful for checking the integrity of the database, even before t
...

💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1388479896)
Yes, because the genesis block.

💬 kevkevinpal commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#discussion_r1388491212)
running this command
`grep -nr --text "\-present" ./src`

I only see these files using `present`
```
./src/streams.cpp:1:// Copyright (c) 2009-present The Bitcoin Core developers
./src/kernel/mempool_removal_reason.cpp:1:// Copyright (c) 2016-present The Bitcoin Core developers
./src/kernel/mempool_removal_reason.h:1:// Copyright (c) 2016-present The Bitcoin Core developers
```

if this is something we're going towards would we want to update the copy right of all files whenever there
...

🤔 pablomartin4btc reviewed a pull request: "[do not merge] validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-1723381054)
Tested `loadtxoutset` a few times (pending IBD to finish) with `coinstatsindex` & `blockfilterindex`, and without them. I've also validated that the `sha256sum` corresponds to the one obtained from the downloaded file.

Now, having a pruned node with IBD completed, from which I got the `m_assumeutxo_data` mentioned in my previous review, when I run `./contrib/devtools/utxo_snapshot.sh` with a filename in order to perform the actual `dumptxoutset`within the script, I got a node crash with `ER
...

💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#discussion_r1388514996)
The scripted-diff for all files should be easy to rebase, see the thread: https://github.com/bitcoin/bitcoin/pull/26817#issuecomment-1433685289

Let me know if I should drop it here and leave it for later. I don't really mind either way, as I mostly care about not running into intermittent issues.

💬 kevkevinpal commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1804551615)
lgtm ACK [44445ae](https://github.com/bitcoin/bitcoin/pull/28831/commits/44445ae8f1123c3affdcc0dbd7b3830eff5548ef)

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388185765)
nit: I think this needs to be included in `validation.h` instead of here since `ChainstateManager::m_interrupt` is a `const util::SignalInterrupt&`

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388253872)
nit: would ~`interrupt`~ -> `shutdown` be more appropriate and consistent name?

(same for `node::AbortNode()`)