💬 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)
📝 KristijanSajenko opened a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)
(https://github.com/bitcoin/bitcoin/pull/30954)
📝 fanquake locked a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...