💬 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?
...
(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`.
(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?
(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 :
```
(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`.
(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?
(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.
(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?
(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?
(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
(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.
(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
...
(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
...
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1804674780)
Thank you for the review @stickies-v!
Updated 2be5e799ba623f969f5ffc59667a5bc6799df290 -> fc710d51b8a64d718dd6d1f66c5eb092cb5f86e9 ([simplifyMemPoolInteractions_12](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_12) -> [simplifyMemPoolInteractions_13](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_12..simplifyMemPoolInteractions_13))
* Addressed @s
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1804674780)
Thank you for the review @stickies-v!
Updated 2be5e799ba623f969f5ffc59667a5bc6799df290 -> fc710d51b8a64d718dd6d1f66c5eb092cb5f86e9 ([simplifyMemPoolInteractions_12](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_12) -> [simplifyMemPoolInteractions_13](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_12..simplifyMemPoolInteractions_13))
* Addressed @s
...
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1804732462)
> That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"
@luke-jr It is a reason, but you don't have to agree with it ;) If we want to move the `contrib` scripts outside of the bitcoin repo that sounds like a pretty big change that would require some conceptual review that is outside of the scope of the asmap project. If there is wider conceptual agreement on this I am o
...
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1804732462)
> That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"
@luke-jr It is a reason, but you don't have to agree with it ;) If we want to move the `contrib` scripts outside of the bitcoin repo that sounds like a pretty big change that would require some conceptual review that is outside of the scope of the asmap project. If there is wider conceptual agreement on this I am o
...
📝 pablomartin4btc opened a pull request: "test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698"
(https://github.com/bitcoin/bitcoin/pull/28835)
This is a follow-up on the invalid hash dump fix #28698, [suggested](https://github.com/bitcoin/bitcoin/pull/28698#pullrequestreview-1698178157) by @theStack and agreed by @Sjors and @ryanofsky.
(https://github.com/bitcoin/bitcoin/pull/28835)
This is a follow-up on the invalid hash dump fix #28698, [suggested](https://github.com/bitcoin/bitcoin/pull/28698#pullrequestreview-1698178157) by @theStack and agreed by @Sjors and @ryanofsky.
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388646431)
done
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388646431)
done
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388646895)
Taken, but reformatted further with f-strings
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388646895)
Taken, but reformatted further with f-strings
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388647127)
done
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388647127)
done
💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388653133)
Dropped it for now
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1388653133)
Dropped it for now
💬 luke-jr commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1804813578)
re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1804813578)
re-ACK d95dde9441fb791046394ed3784a840a54ef2ab9