💬 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 :
```

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388531074)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts":
It’s a bit confusing that you take an input `tx1` here which is the 'hex' string of the raw transaction, but then assign the resulting 'txid' back to `tx1`.

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388510399)
In 28a7e883c2f9249213fddbe24948ec5bb90b0fad "wallet: track mempool conflicts":
Could you perhaps explain what the purpose of mining ten blocks here is?

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388493685)
In 28a7e883c2f9249213fddbe24948ec5bb90b0fad "wallet: track mempool conflicts":

Note to self and other reviewers: `TxSpends` is an `unordered_multimap` which permits multiple entries to have the same key, and `equal_range(…)` returns the range of iterators that match the given key denominated in first iterator to first iterator after the range.

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388532260)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts":
Since `tx2` replaced `tx1`, why would `unspents[0]` be considered spent here?

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388547724)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts":
`tx1_conflict` should displace `tx1`, but why does Bob not have 3 again now? Shouldn’t Bob know about `tx1`, `tx2`, and `tx3` before, and then `tx1_conflict`, `tx2`, and `tx3` after?

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388545575)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts":
I’m having trouble keeping track of all the things going on here. Perhaps you could add in these comments which transactions each node now knows about

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1388533849)
Yeah, this could be a problem if you only run one of the tests separately instead of the whole suite. Then that test would fail, since it’s setup depends on the prior test having been run before.

💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1804651014)
> Thinking about this more, it feels like the question to be answered is perhaps "is the `asm` field supposed to be human or machine readable?"

I think it's human readable? For machines we could just give the raw hex script and not bother with an asm format.

Anyway, how about going for something more left field and having `decodescript 0511121314150457c74942` output `[1112131415] 1112131415` where the square brackets indicated "quoted hexidecimal", and values without square brackets are al
...