💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1771952837)
And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn't feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1771952837)
And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn't feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.
💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2369170805)
> You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
I don’t deny it that you need only one low-latency connection to run a pretty effective tx spamming attack. Though, in my understanding, you’re poi
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2369170805)
> You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)
I don’t deny it that you need only one low-latency connection to run a pretty effective tx spamming attack. Though, in my understanding, you’re poi
...
💬 achow101 commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2369179390)
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2369179390)
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
🚀 achow101 merged a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409)
(https://github.com/bitcoin/bitcoin/pull/30409)
💬 achow101 commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2369211315)
ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2369211315)
ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee
💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369236572)
> Can you explain this a bit better?
Yeah, my statement was wrong. I think I managed to confuse myself with some wrongfully placed debug logging. I have fixed the description.
> > Well, the approach seems to be similar with pruning:
>
> Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU.
Updated the error message and realized that he same issue exists in `rescanblockchai
...
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369236572)
> Can you explain this a bit better?
Yeah, my statement was wrong. I think I managed to confuse myself with some wrongfully placed debug logging. I have fixed the description.
> > Well, the approach seems to be similar with pruning:
>
> Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU.
Updated the error message and realized that he same issue exists in `rescanblockchai
...
🚀 achow101 merged a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678)
(https://github.com/bitcoin/bitcoin/pull/30678)
💬 fjahr commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369292131)
@achow101 do you have thoughts on descriptor import/rescan behavior with missing blocks because of ongoing assumeutxo background sync?
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2369292131)
@achow101 do you have thoughts on descriptor import/rescan behavior with missing blocks because of ongoing assumeutxo background sync?
🤔 theuni reviewed a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2323243428)
Concept ACK, and scripted-diff lgtm ACK.
Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2323243428)
Concept ACK, and scripted-diff lgtm ACK.
Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2323147329)
I think I need to rebase this now that #30409 is merged, so I'll also work on improving comments here as suggested in recent reviews.
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2323147329)
I think I need to rebase this now that #30409 is merged, so I'll also work on improving comments here as suggested in recent reviews.
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771976019)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747
> In [246fd7f](https://github.com/bitcoin/bitcoin/commit/246fd7faae85e871ba78101f1dee8d795437b8f6) "multiprocess: Add serialization code for vector"
>
> Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
tl;dr: Adding Span constructors would not eliminate the need for this builder. They would just allow defining a reader that's as generic as this builder
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771976019)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747
> In [246fd7f](https://github.com/bitcoin/bitcoin/commit/246fd7faae85e871ba78101f1dee8d795437b8f6) "multiprocess: Add serialization code for vector"
>
> Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
tl;dr: Adding Span constructors would not eliminate the need for this builder. They would just allow defining a reader that's as generic as this builder
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1772043669)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209
> There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
I guess a way to
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1772043669)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209
> There are some places where a `BlockValidationState` is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a `BlockValidateState` were serialized then deserialized, these assertions would be hit. I don't know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
I guess a way to
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771987455)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488
> This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
This is saying defining a `CustomReadField` function corresponding to this `CustomBuildField` function is unnecessary. Not that this `CustomBuildField` is unnecessary.
Specifically a `CustomReadField` function that converts a `::capnp::Data` value into a `vector<ch
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771987455)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488
> This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
This is saying defining a `CustomReadField` function corresponding to this `CustomBuildField` function is unnecessary. Not that this `CustomBuildField` is unnecessary.
Specifically a `CustomReadField` function that converts a `::capnp::Data` value into a `vector<ch
...
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772164812)
Yes, I often see peers with no services, both new connections still setting up and longer-lasting ones, where getpeerinfo returns
```
"services": "0000000000000000",
"servicesnames": [
],
```
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772164812)
Yes, I often see peers with no services, both new connections still setting up and longer-lasting ones, where getpeerinfo returns
```
"services": "0000000000000000",
"servicesnames": [
],
```
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772171550)
(`continue` could probably `break` here instead)
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772171550)
(`continue` could probably `break` here instead)
💬 achow101 commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772081225)
In cbb053c5f6fa63b08fe8fb200b143cca64fc0626 "net: Add PCP and NATPMP implementation"
Lifetime is 4 bytes.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772081225)
In cbb053c5f6fa63b08fe8fb200b143cca64fc0626 "net: Add PCP and NATPMP implementation"
Lifetime is 4 bytes.
💬 achow101 commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020)
In e02030432b77abbf27bb4f67d879d3ad6d6366e6 "net: Add netif utility"
It seems a little bit odd to me to get the local addresses this way when the Windows header already being used by this file provides a [`GetAdaptersAddresses`](https://learn.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getadaptersaddresses) function that appears to be equivalent to `getifaddrs`.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020)
In e02030432b77abbf27bb4f67d879d3ad6d6366e6 "net: Add netif utility"
It seems a little bit odd to me to get the local addresses this way when the Windows header already being used by this file provides a [`GetAdaptersAddresses`](https://learn.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getadaptersaddresses) function that appears to be equivalent to `getifaddrs`.
💬 achow101 commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772139009)
In cbb053c5f6fa63b08fe8fb200b143cca64fc0626 "net: Add PCP and NATPMP implementation"
`CNetAddr` has a `SetLegacyIPv6` method which does the same thing. Could use that instead of duplicating implementation.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772139009)
In cbb053c5f6fa63b08fe8fb200b143cca64fc0626 "net: Add PCP and NATPMP implementation"
`CNetAddr` has a `SetLegacyIPv6` method which does the same thing. Could use that instead of duplicating implementation.
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772215530)
Two examples right now:
```
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id address version
in onion nwl 1 10 10 . 0 16818 127.0.0.1:63651 70016/Satoshi:26.1.0/
in cjdns nwcl2 2 282 295 5 3 4 377 246 15919 [fccb:248:11
...
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772215530)
Two examples right now:
```
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id address version
in onion nwl 1 10 10 . 0 16818 127.0.0.1:63651 70016/Satoshi:26.1.0/
in cjdns nwcl2 2 282 295 5 3 4 377 246 15919 [fccb:248:11
...
💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772270099)
Sorry, I wasn't as clear as I could have been. It seems possible to have no services (e.g. `servicesnames` has no elements, in which case the for loop wouldn't be entered, right?). What I meant to ask is if we think we'll see `servicesnames` with some non-zero number of elements, with at least one element satisfying the check to `isNull()`? In other words, does the `continue` ever actually happen?
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772270099)
Sorry, I wasn't as clear as I could have been. It seems possible to have no services (e.g. `servicesnames` has no elements, in which case the for loop wouldn't be entered, right?). What I meant to ask is if we think we'll see `servicesnames` with some non-zero number of elements, with at least one element satisfying the check to `isNull()`? In other words, does the `continue` ever actually happen?
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772309840)
Good point, SGTM to add a CHECK_NONFATAL there.
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1772309840)
Good point, SGTM to add a CHECK_NONFATAL there.