💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169161201)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
It seems like the code in this lambda would be simpler it avoided calling GetTxDepthInMainChain. The following should be equivalent:
```c++
// If the orig tx was not in block/mempool, none of its spends can be.
assert(!wtx.isConfirmed());
assert(!wtx.InMempool());
// If already conflicted or abandoned, no need to set abandoned.
if (!wtx.isConflicted() && !wtx.isAbandon
...
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169161201)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
It seems like the code in this lambda would be simpler it avoided calling GetTxDepthInMainChain. The following should be equivalent:
```c++
// If the orig tx was not in block/mempool, none of its spends can be.
assert(!wtx.isConfirmed());
assert(!wtx.InMempool());
// If already conflicted or abandoned, no need to set abandoned.
if (!wtx.isConflicted() && !wtx.isAbandon
...
💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169145950)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think as long as this code is still calling GetTxDepthInMainChain, it would be better to keep the 3 comments that were here instead of deleting them, because they explain why the asserts are valid and why conflicted transactions are ignored
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169145950)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think as long as this code is still calling GetTxDepthInMainChain, it would be better to keep the 3 comments that were here instead of deleting them, because they explain why the asserts are valid and why conflicted transactions are ignored
💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169166456)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This also seems like a useful comment. Would keep instead of deleting
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169166456)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This also seems like a useful comment. Would keep instead of deleting
💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169172208)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This code pretty opaque, would suggest copying the previous comment here "Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too" or something equivalent.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169172208)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This code pretty opaque, would suggest copying the previous comment here "Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too" or something equivalent.
💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169173396)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
Would suggest keeping the previous comment here instead of deleting it. It's not obvious what the check means or what's happening in the block:
```c++
// Block is 'more conflicted' than current confirm; update.
// Mark transaction as conflicted with this block.
```
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169173396)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
Would suggest keeping the previous comment here instead of deleting it. It's not obvious what the check means or what's happening in the block:
```c++
// Block is 'more conflicted' than current confirm; update.
// Mark transaction as conflicted with this block.
```
💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601)
This non-deterministic test could fail intermittently, if `i2p_addr` collides with `addr1` (I tried it, and it happened to me after ~4000 runs).
It would be good to change the order, such that we first add an address to tried (via new), and only add another addr to new when the first has been moved over to tried.
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601)
This non-deterministic test could fail intermittently, if `i2p_addr` collides with `addr1` (I tried it, and it happened to me after ~4000 runs).
It would be good to change the order, such that we first add an address to tried (via new), and only add another addr to new when the first has been moved over to tried.
🤔 mzumsande reviewed a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1388792230)
Code Review ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
I reviewed the PR again - although I'm a coauthor, so not sure about the etiquette of ack'ing.
I think there is a small bug in the non-deterministic test that could make it fail on rare occasions and should be fixed - here or in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1388792230)
Code Review ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
I reviewed the PR again - although I'm a coauthor, so not sure about the etiquette of ack'ing.
I think there is a small bug in the non-deterministic test that could make it fail on rare occasions and should be fixed - here or in a follow-up.
💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169178603)
nit: mentioning the "empty pair" only in the first case makes it sound as if this wasn't one of the possible outcomes in the second case (but it is).
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169178603)
nit: mentioning the "empty pair" only in the first case makes it sound as if this wasn't one of the possible outcomes in the second case (but it is).
💬 theuni commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511986632)
Ok, yeah, I guess my question was: why isn't this slipping into a "CC=clang" build. But point taken that's not really a supported combo.
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511986632)
Ok, yeah, I guess my question was: why isn't this slipping into a "CC=clang" build. But point taken that's not really a supported combo.
💬 satsie commented on issue "Add per message stats to getnettotals rpc":
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1512090103)
Hi, I'm back again :laughing:
Here is the latest code I have: https://github.com/satsie/bitcoin/pull/5
It incorporates a bunch of feedback I got from the code I previously posted. The changes are mostly under the hood and have all been documented in the new PR's description for anyone that is curious. Design-wise, the only noteworthy thing is 'direction' is now available as a "aggregate type" (the term I am now using instead of "filter"). So now, results can be aggregated by:
- direction
...
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1512090103)
Hi, I'm back again :laughing:
Here is the latest code I have: https://github.com/satsie/bitcoin/pull/5
It incorporates a bunch of feedback I got from the code I previously posted. The changes are mostly under the hood and have all been documented in the new PR's description for anyone that is curious. Design-wise, the only noteworthy thing is 'direction' is now available as a "aggregate type" (the term I am now using instead of "filter"). So now, results can be aggregated by:
- direction
...
📝 furszy opened a pull request: "wallet: bugfix, 'AvailableCoins' invalid output type set for raw PK outputs"
(https://github.com/bitcoin/bitcoin/pull/27478)
`AvailableCoins` sets raw pubkey outputs with an `OutputType::UNKNOWN` type instead of `OutputType::LEGACY`.
Gladly, no type unknown outputs are skipped during coin selection, nor anything different is done with them.
It is just an inconsistency.
(https://github.com/bitcoin/bitcoin/pull/27478)
`AvailableCoins` sets raw pubkey outputs with an `OutputType::UNKNOWN` type instead of `OutputType::LEGACY`.
Gladly, no type unknown outputs are skipped during coin selection, nor anything different is done with them.
It is just an inconsistency.
💬 achow101 commented on pull request "wallet: bugfix, 'AvailableCoins' invalid output type set for raw PK outputs":
(https://github.com/bitcoin/bitcoin/pull/27478#issuecomment-1512107265)
I'm not sure if that's really a bug. I don't think we actually classify p2pk outputs as being legacy since they don't have an address. Really `legacy` is talking about address type.
Additionally, in descriptors, `pk()` descriptors are treated as having no output type, rather than `unknown` or `legacy`.
(https://github.com/bitcoin/bitcoin/pull/27478#issuecomment-1512107265)
I'm not sure if that's really a bug. I don't think we actually classify p2pk outputs as being legacy since they don't have an address. Really `legacy` is talking about address type.
Additionally, in descriptors, `pk()` descriptors are treated as having no output type, rather than `unknown` or `legacy`.
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1512128741)
thanks for the reviews! I'll address the outstanding comments in a followup
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1512128741)
thanks for the reviews! I'll address the outstanding comments in a followup
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169300130)
🤦♀️ yup, agreed. will do in follow-up
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169300130)
🤦♀️ yup, agreed. will do in follow-up
📝 sipa opened a pull request: "BIP324: ElligatorSwift integrations"
(https://github.com/bitcoin/bitcoin/pull/27479)
This replaces #23432 and part of #23561.
This PR introduces all of the ElligatorSwift-related changes (libsecp256k1 updates, generation, decoding, ECDH, tests, fuzzing, benchmarks) in. These were previously split over two PRs, but as the libsecp256k1 code has ECDH operations implemented directly on the ellswift encoded public keys, it feels more natural to merge them.
Some of the code is structured a bit differently, as some further simplifications were possible.
(https://github.com/bitcoin/bitcoin/pull/27479)
This replaces #23432 and part of #23561.
This PR introduces all of the ElligatorSwift-related changes (libsecp256k1 updates, generation, decoding, ECDH, tests, fuzzing, benchmarks) in. These were previously split over two PRs, but as the libsecp256k1 code has ECDH operations implemented directly on the ellswift encoded public keys, it feels more natural to merge them.
Some of the code is structured a bit differently, as some further simplifications were possible.
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#issuecomment-1512175200)
I'm in the process of taking over the BIP324 PRs from @dhruv.
Note for reviewers:
* Previously, the EllSwift encoding/decoding/creation was in #23432, while the ECDH logic was in #23561. I've opted here to merge all EllSwift-related functionality into 1 PR, as the integration is now tighter at the libsecp256k1 too compared to when the PRs were first opened (libsecp256k1 has functions for directly performing ECDH on the EllSwift-encoded keys).
* `EllSwiftPubKey` is now a proper class that ex
...
(https://github.com/bitcoin/bitcoin/pull/27479#issuecomment-1512175200)
I'm in the process of taking over the BIP324 PRs from @dhruv.
Note for reviewers:
* Previously, the EllSwift encoding/decoding/creation was in #23432, while the ECDH logic was in #23561. I've opted here to merge all EllSwift-related functionality into 1 PR, as the integration is now tighter at the libsecp256k1 too compared to when the PRs were first opened (libsecp256k1 has functions for directly performing ECDH on the EllSwift-encoded keys).
* `EllSwiftPubKey` is now a proper class that ex
...
💬 furszy commented on pull request "wallet: bugfix, 'AvailableCoins' invalid output type set for raw PK outputs":
(https://github.com/bitcoin/bitcoin/pull/27478#issuecomment-1512179805)
> I'm not sure if that's really a bug so long as we aren't excluding them from coin selection. I don't think we actually classify p2pk outputs as being legacy since they don't have an address. Really legacy is talking about address type.
Yeah, so far we aren't excluding them from coin selection but hmm, the more I think about this, the odder is turns for me.
Like, we mix this "legacy" output type usage for the address encoding and also to create and handle the legacy spkm. Because, in parall
...
(https://github.com/bitcoin/bitcoin/pull/27478#issuecomment-1512179805)
> I'm not sure if that's really a bug so long as we aren't excluding them from coin selection. I don't think we actually classify p2pk outputs as being legacy since they don't have an address. Really legacy is talking about address type.
Yeah, so far we aren't excluding them from coin selection but hmm, the more I think about this, the odder is turns for me.
Like, we mix this "legacy" output type usage for the address encoding and also to create and handle the legacy spkm. Because, in parall
...
💬 RandyMcMillan commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1512186089)
utACK d8434da
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1512186089)
utACK d8434da
💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1169360367)
hmm, thinking more about this, I didn't see the whole picture, because everything happens in `ThreadSocketHandler` - after ` CConnman::SocketHandler() -> CConnman::SocketHandlerListening()`, `CConnman::DisconnectNodes()` is called, so it's not possible to accept new connections in between - however, `CConnman::SocketHandlerListening()` calls `AcceptConnection()` in a loop for multiple `ListenSocket`s, so if we have multiple listening sockets and an attacker would connect to more than one at the
...
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1169360367)
hmm, thinking more about this, I didn't see the whole picture, because everything happens in `ThreadSocketHandler` - after ` CConnman::SocketHandler() -> CConnman::SocketHandlerListening()`, `CConnman::DisconnectNodes()` is called, so it's not possible to accept new connections in between - however, `CConnman::SocketHandlerListening()` calls `AcceptConnection()` in a loop for multiple `ListenSocket`s, so if we have multiple listening sockets and an attacker would connect to more than one at the
...
💬 fanquake commented on pull request "BIP324: CKey encode/decode to elligator-swift":
(https://github.com/bitcoin/bitcoin/pull/23432#issuecomment-1512280285)
Closing for now. See #27479.
(https://github.com/bitcoin/bitcoin/pull/23432#issuecomment-1512280285)
Closing for now. See #27479.