💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1660405202)
Rebased on the latest #https://github.com/bitcoin/bitcoin/pull/28060
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1660405202)
Rebased on the latest #https://github.com/bitcoin/bitcoin/pull/28060
💬 Sjors commented on pull request "rpc, wallet: addhdseed, infer seed when importing descriptor with xpub ":
(https://github.com/bitcoin/bitcoin/pull/23544#issuecomment-1660421353)
This is very out of date and it's not on my priority list at the moment. Happy to review if someone else takes it over.
(https://github.com/bitcoin/bitcoin/pull/23544#issuecomment-1660421353)
This is very out of date and it's not on my priority list at the moment. Happy to review if someone else takes it over.
✅ Sjors closed a pull request: "rpc, wallet: addhdseed, infer seed when importing descriptor with xpub "
(https://github.com/bitcoin/bitcoin/pull/23544)
(https://github.com/bitcoin/bitcoin/pull/23544)
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280715228)
> Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
The mempool value is incremented when TrAddedTMP is queued, not when it's eventually run -- the increment happens first, then the result of the increment is passed through and queued up. So I don't think there's any racy here.
> Perhaps it makes sense to use `m_pool.GetSequence() + 1` here?
I think it would make more sense to change `info_for_relay`?
```
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280715228)
> Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
The mempool value is incremented when TrAddedTMP is queued, not when it's eventually run -- the increment happens first, then the result of the increment is passed through and queued up. So I don't think there's any racy here.
> Perhaps it makes sense to use `m_pool.GetSequence() + 1` here?
I think it would make more sense to change `info_for_relay`?
```
...
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280721691)
I switched to using util::Result in my last rebase a few months ago.
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280721691)
I switched to using util::Result in my last rebase a few months ago.
💬 MarcoFalke commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280757943)
Pretty sure this is wrong, as you are not checking `!res.value()`. (Previously it threw, now it silently continues. Now it only throws when no `ExternalSignerScriptPubKeyMan` is available.)
This can be fixed non-confusingly by using `Result<void>`
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280757943)
Pretty sure this is wrong, as you are not checking `!res.value()`. (Previously it threw, now it silently continues. Now it only throws when no `ExternalSignerScriptPubKeyMan` is available.)
This can be fixed non-confusingly by using `Result<void>`
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280759603)
> Doesn't that mean there are races? (There is no guarantee that TransactionAddedToMempool() will always run in the near future)?
I meant `TransactionAddedToMempool` event happening, not the validation interface clients' callbacks. IIUC the event firing on main signals (which calls `GetAndIncrementSequence()`) happens in validation before `ProcessTransaction` returns:
https://github.com/bitcoin/bitcoin/blob/e5a9f2fb62dc4db6cad21b2997d96a881ea64125/src/validation.cpp#L1234
The queued callb
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280759603)
> Doesn't that mean there are races? (There is no guarantee that TransactionAddedToMempool() will always run in the near future)?
I meant `TransactionAddedToMempool` event happening, not the validation interface clients' callbacks. IIUC the event firing on main signals (which calls `GetAndIncrementSequence()`) happens in validation before `ProcessTransaction` returns:
https://github.com/bitcoin/bitcoin/blob/e5a9f2fb62dc4db6cad21b2997d96a881ea64125/src/validation.cpp#L1234
The queued callb
...
👍 jamesob approved a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1557204259)
reACK fa633aa6906f3b130b691568bcd20b2b76bb1cbb ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tiny diff since last review; moving `ftell` out of a loop.
<details><summary>Range-diff</summary>
```diff
6: faebf00dbf ! 6: fa633aa690 streams: Teach AutoFile how to XOR
@@ src/streams.cpp: void AutoFile::ignore(size_t nSize)
+ throw std::ios_base::failure("AutoFile::write: writ
...
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1557204259)
reACK fa633aa6906f3b130b691568bcd20b2b76bb1cbb ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tiny diff since last review; moving `ftell` out of a loop.
<details><summary>Range-diff</summary>
```diff
6: faebf00dbf ! 6: fa633aa690 streams: Teach AutoFile how to XOR
@@ src/streams.cpp: void AutoFile::ignore(size_t nSize)
+ throw std::ios_base::failure("AutoFile::write: writ
...
💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1660491860)
concept ACK
I read the linked cirrus guide about persistent workers, everything makes sense with these changes. Two questions:
1. Is the persistent worker hosted by you? Are there any documented details about it?
2. Why does the `previous releases` task need persistent worker?
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1660491860)
concept ACK
I read the linked cirrus guide about persistent workers, everything makes sense with these changes. Two questions:
1. Is the persistent worker hosted by you? Are there any documented details about it?
2. Why does the `previous releases` task need persistent worker?
💬 MarcoFalke commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1660492118)
Needs rebase
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1660492118)
Needs rebase
💬 MarcoFalke commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-1660494465)
Maybe mark as draft for as long as CI is red and this is based on something else?
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-1660494465)
Maybe mark as draft for as long as CI is red and this is based on something else?
🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930)
> What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.
Right now, this will hardly happen. The reason is `CheckForStaleTipAndEvictPeers`. The logic is as follows:
When the node doesn't update the tip for a period longer than 30 minutes (`TipMayBeStale()`), it triggers the extra outbound connection process. Which instructs the net layer to
...
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930)
> What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.
Right now, this will hardly happen. The reason is `CheckForStaleTipAndEvictPeers`. The logic is as follows:
When the node doesn't update the tip for a period longer than 30 minutes (`TipMayBeStale()`), it triggers the extra outbound connection process. Which instructs the net layer to
...
💬 MarcoFalke commented on pull request "blockstorage: Drop legacy -txindex check":
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1660503311)
Thanks, fixed up the scripted-diff
(https://github.com/bitcoin/bitcoin/pull/28195#issuecomment-1660503311)
Thanks, fixed up the scripted-diff
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280780213)
Ah I didn't see the last message.
> I think it would make more sense to change info_for_relay?
sounds good to me
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280780213)
Ah I didn't see the last message.
> I think it would make more sense to change info_for_relay?
sounds good to me
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660512698)
Reposting my reply to this identical message on the mailing list:
On Mon, Jul 31, 2023 at 03:13:19AM -0700, Daniel Lipshitz wrote:
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660512698)
Reposting my reply to this identical message on the mailing list:
On Mon, Jul 31, 2023 at 03:13:19AM -0700, Daniel Lipshitz wrote:
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC
...
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660520660)
On Sun, Jul 30, 2023 at 02:18:25PM -0700, Matt Corallo wrote:
> > This is helpful for second layer protocols, including Lightning.
>
> I'm not aware of any second layer protocols that are improved by having full-rbf more broadly available.
@ariard provided an example above.
See also: https://petertodd.org/2023/fullrbf-multiparty-protocols
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660520660)
On Sun, Jul 30, 2023 at 02:18:25PM -0700, Matt Corallo wrote:
> > This is helpful for second layer protocols, including Lightning.
>
> I'm not aware of any second layer protocols that are improved by having full-rbf more broadly available.
@ariard provided an example above.
See also: https://petertodd.org/2023/fullrbf-multiparty-protocols
💬 fanquake commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1660543177)
Also not sure. This might remove the duplicate `AmountFromValue` from bitcoin-tx, but instead we end up with an extra wrapper function in rpc/util.h?
Generally, I think that anytime you're putting the return type of a function into it's name, something is going a bit wrong. Given in this case it's because we've already got a function called `AmountFromValue`, someone might ask why not just refactor that to return `util::Result`, instead of adding a new layer of indirection.
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1660543177)
Also not sure. This might remove the duplicate `AmountFromValue` from bitcoin-tx, but instead we end up with an extra wrapper function in rpc/util.h?
Generally, I think that anytime you're putting the return type of a function into it's name, something is going a bit wrong. Given in this case it's because we've already got a function called `AmountFromValue`, someone might ask why not just refactor that to return `util::Result`, instead of adding a new layer of indirection.
👍 willcl-ark approved a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1557235174)
ACK 1e65aae806
Left one more observation since last time related to the mutex locking. I'm not sure how important the Recusive Mutex-removal project is in general and whether it's worth changing though.
I would also be happy to re-review if the nits were addressed in here FWIW :)
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1557235174)
ACK 1e65aae806
Left one more observation since last time related to the mutex locking. I'm not sure how important the Recusive Mutex-removal project is in general and whether it's worth changing though.
I would also be happy to re-review if the nits were addressed in here FWIW :)
💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1280786062)
```suggestion
AssertLockHeld(m_nodes_mutex);
```
We lock `m_nodes_mutex` here, but this is only called from inside `ForEachNode`, which itself has already locked the same mutex. It's no problem because `m-Nodes_mutex` is Recursive, but as there is a passive [effort to replace Recursive Mutexes](https://github.com/bitcoin/bitcoin/issues/19303), if you do end up touching this again perhaps you could assert the lock is held at runtime with `AssertLockHeld` and decorate header declaration w
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1280786062)
```suggestion
AssertLockHeld(m_nodes_mutex);
```
We lock `m_nodes_mutex` here, but this is only called from inside `ForEachNode`, which itself has already locked the same mutex. It's no problem because `m-Nodes_mutex` is Recursive, but as there is a passive [effort to replace Recursive Mutexes](https://github.com/bitcoin/bitcoin/issues/19303), if you do end up touching this again perhaps you could assert the lock is held at runtime with `AssertLockHeld` and decorate header declaration w
...
💬 fanquake commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1660554671)
Going to suggest closing #28189 and just dealing with the final changes here. We can't change commit messages after the fact, so if that's a concern for some, we should correct them. I think with range-diff, and two reviewers already commited to re-ACKing, we can integrate the final changes and get this merged without issue.
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1660554671)
Going to suggest closing #28189 and just dealing with the final changes here. We can't change commit messages after the fact, so if that's a concern for some, we should correct them. I think with range-diff, and two reviewers already commited to re-ACKing, we can integrate the final changes and get this merged without issue.