💬 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()`)

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388407695)
This interface of dealing with shutdown failures feels a bit awkward to me. It's verbose and quite difficult to parse. These shutdown failures aren't really related to the callsite, and it looks like mostly we just log and then ignore them anyway.

What do you think about adding failure callbacks to `SignalInterrupt`? It still allows the caller to individually handle shutdown failures when it makes sense (still returning bools), but in most cases the generic callback will probably suffice?

...

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388335574)
nit: on Windows, I don't think the failure code path is ever reached, since `SignalInterrupt::operator()()` always returns true for `#ifdef WIN32`. But perhaps that would be overly relying on the implementation of `SignalInterrupt`.

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1387276842)
In 5107e4e730a150068c5a214020a6393d2ba33aa8 "wallet: use CWalletTx member functions to determine tx state":

While I see why `TxStateConflicted` would be excluded here, it is not quite obvious to me why your replacement is equivalent. Could you perhaps expand a bit on this in a comment or the commit message?

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1387279137)
In 920f16c9a11d167444f509a9fd2f0244f1f635eb "scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted":

I was considering whether there might be a way to better express that the transaction is in conflict with a confirmed transaction. How about:

```suggestion
wtx.state<TxStateConfirmedConflict>() ? wtx.state<TxStateConfirmedConflict>()->conflicting_block_height :
```