💬 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.
💬 theStack commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2369695246)
> Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
Sounds good to me, done. Both the scripted diff and the patch are now a bit larger, as also some comments/strings have to be adapted, and the .h.in filename needs to be adapted as well, but review should still be simple.
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2369695246)
> Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?
Sounds good to me, done. Both the scripted diff and the patch are now a bit larger, as also some comments/strings have to be adapted, and the .h.in filename needs to be adapted as well, but review should still be simple.
💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2369696567)
My Guix build of the demo [branch](https://github.com/hebasto/bitcoin/commits/240921-guix-mp.DEMO/) except for `x86_64-w64-mingw32`:
```
aarch64
e31ab0eeff88048301f113731a9b829df63e7db2c6a83c3a54e14b6f423aa4a8 guix-build-d8ec933456bc/output/aarch64-linux-gnu/SHA256SUMS.part
77f4a9481b4ce7df26549c6001384e066a1de3cb1b81ba950c29cc41b0b5c058 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoin-d8ec933456bc-aarch64-linux-gnu-debug.tar.gz
ee471d630ea0eb71c2040a3b9e408c4f1709ee29d2da4ecbc193
...
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2369696567)
My Guix build of the demo [branch](https://github.com/hebasto/bitcoin/commits/240921-guix-mp.DEMO/) except for `x86_64-w64-mingw32`:
```
aarch64
e31ab0eeff88048301f113731a9b829df63e7db2c6a83c3a54e14b6f423aa4a8 guix-build-d8ec933456bc/output/aarch64-linux-gnu/SHA256SUMS.part
77f4a9481b4ce7df26549c6001384e066a1de3cb1b81ba950c29cc41b0b5c058 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoin-d8ec933456bc-aarch64-linux-gnu-debug.tar.gz
ee471d630ea0eb71c2040a3b9e408c4f1709ee29d2da4ecbc193
...
💬 maflcko commented on pull request "test: Use shell builtins in run_command test case":
(https://github.com/bitcoin/bitcoin/pull/30952#issuecomment-2370288142)
review ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539
Seems fine to replace the `python3` dependency with `sh`, assuming that `sh` is available on more systems (at least on all systems where `ls` was previously available)
(https://github.com/bitcoin/bitcoin/pull/30952#issuecomment-2370288142)
review ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539
Seems fine to replace the `python3` dependency with `sh`, assuming that `sh` is available on more systems (at least on all systems where `ls` was previously available)