💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509422876)
Why do we get 14?
Tried adding...
```C++
{
AutoFile test{raw_file("rb"), obfuscation};
BOOST_CHECK_EQUAL(test.size(), 14);
}
```
...just after this block before digging into the data but it fails with `size()` suddenly returning 7 (which would be what we expect: 2 vector compact size bytes + 2+3 bytes of data).
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509422876)
Why do we get 14?
Tried adding...
```C++
{
AutoFile test{raw_file("rb"), obfuscation};
BOOST_CHECK_EQUAL(test.size(), 14);
}
```
...just after this block before digging into the data but it fails with `size()` suddenly returning 7 (which would be what we expect: 2 vector compact size bytes + 2+3 bytes of data).
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509435441)
This last `seek()` can be removed and all unit tests still pass (259 non-skipped functional tests all passed as well, no failures in my non-`--extended` run), could add a test that covers this so we don't spawn any mutation testing mutants.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509435441)
This last `seek()` can be removed and all unit tests still pass (259 non-skipped functional tests all passed as well, no failures in my non-`--extended` run), could add a test that covers this so we don't spawn any mutation testing mutants.
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509450577)
nit: Should be moved above `DataStream::GetMemoryUsage()` so it's kept together with other `AutoFile` methods. Maybe after `AutoFile::tell` to keep some consistency with header file.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509450577)
nit: Should be moved above `DataStream::GetMemoryUsage()` so it's kept together with other `AutoFile` methods. Maybe after `AutoFile::tell` to keep some consistency with header file.
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509464158)
remark: Good that it is non-`const` as it is not thread-safe. :+1:
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509464158)
remark: Good that it is non-`const` as it is not thread-safe. :+1:
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511238194)
remark: Tried...
```shell
₿ printf fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b36fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653bc939d4327f287e8b4d1f8aff33176787cb0ff7cb28e3fdaef0f8f47357f801c9f7ff7a99f7f9c9f99de7f3156ae00f23eb27a303bc486aa3ccc31ec19394c2f8a53dddea3cc56257f3b7e9b1f488be9c1137db823759aa4e071eef2e984aaf97b52d5f88d0f373dd190fe45e06efef1df7278be680a73a74c76db4dd910f1d30752c57fe2bc9f079f1a1e1b036c2a69219f11c5e11980a3fa51f4f82d36373de73b1863a8c
...
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511238194)
remark: Tried...
```shell
₿ printf fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b36fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653bc939d4327f287e8b4d1f8aff33176787cb0ff7cb28e3fdaef0f8f47357f801c9f7ff7a99f7f9c9f99de7f3156ae00f23eb27a303bc486aa3ccc31ec19394c2f8a53dddea3cc56257f3b7e9b1f488be9c1137db823759aa4e071eef2e984aaf97b52d5f88d0f373dd190fe45e06efef1df7278be680a73a74c76db4dd910f1d30752c57fe2bc9f079f1a1e1b036c2a69219f11c5e11980a3fa51f4f82d36373de73b1863a8c
...
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511202729)
Instead of implementing `AutoFile::size()`, did you consider using `fs::file_size()` as we do in a few other places?
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511202729)
Instead of implementing `AutoFile::size()`, did you consider using `fs::file_size()` as we do in a few other places?
🤔 Sjors reviewed a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3446684328)
Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_ could itself be split, see inline comment from @l0rinc, since it's still contains a lot of bech32 changes despite the commit name.
My main other suggestion is to drop the first commit (and don't add the network name to error messages).
See inline comment for how to fix the test.
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3446684328)
Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_ could itself be split, see inline comment from @l0rinc, since it's still contains a lot of bech32 changes despite the commit name.
My main other suggestion is to drop the first commit (and don't add the network name to error messages).
See inline comment for how to fix the test.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513222433)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_: the extra `(m)` breaks the test
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513222433)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_: the extra `(m)` breaks the test
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513235396)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
The elegant solution imo is to move these test vectors into a JSON file, see e.g. `rpc_getblockstats.py`.
But I think the current approach is fine. You could also duplicate the body of `test_validateaddress_on_network` into `test_validateaddress_mainnet` and `test_validateaddress_signet` rather than copying the constants. Each for loop could then be preceded with `self.log.debug("Valid mainnet addre
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513235396)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
The elegant solution imo is to move these test vectors into a JSON file, see e.g. `rpc_getblockstats.py`.
But I think the current approach is fine. You could also duplicate the body of `test_validateaddress_on_network` into `test_validateaddress_mainnet` and `test_validateaddress_signet` rather than copying the constants. Each for loop could then be preceded with `self.log.debug("Valid mainnet addre
...
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513250539)
Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 _Bech32 decoding logic_: unrelated whitespace change and random looking `asdas` in the commit message.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513250539)
Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 _Bech32 decoding logic_: unrelated whitespace change and random looking `asdas` in the commit message.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513216866)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.
(nit: typo in commit message)
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513216866)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.
(nit: typo in commit message)
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513196144)
In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 _Include Base58 encoded prefixes in chainparams_: this commit only adds base58 prefixes, silent payments will use bech32m.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513196144)
In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 _Include Base58 encoded prefixes in chainparams_: this commit only adds base58 prefixes, silent payments will use bech32m.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513186197)
In 055dbaed69a2577b967fae3d9b7646ef3b01a254 _Added chaintype user-facing string display_:
This is used as follows in later commits:
> `Invalid Base58 %s address.`
In the original code it would not specify the network:
> `Invalid or unsupported Base58-encoded address.`
I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.
My suggestion would be drop this helper enti
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513186197)
In 055dbaed69a2577b967fae3d9b7646ef3b01a254 _Added chaintype user-facing string display_:
This is used as follows in later commits:
> `Invalid Base58 %s address.`
In the original code it would not specify the network:
> `Invalid or unsupported Base58-encoded address.`
I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.
My suggestion would be drop this helper enti
...
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513273103)
In 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_: splitting bech32 and base58 handling in an earlier commit might also help to focus the current commit more on base58. I still a lot of lines touching bech32 code.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513273103)
In 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_: splitting bech32 and base58 handling in an earlier commit might also help to focus the current commit more on base58. I still a lot of lines touching bech32 code.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513247821)
If this works out of the box it's worth taking, but otherwise I would keep the current approach. Address validation is simple enough that I'm not worried about the node being in a confused state.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513247821)
If this works out of the box it's worth taking, but otherwise I would keep the current approach. Address validation is simple enough that I'm not worried about the node being in a confused state.
💬 laanwj commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2513309951)
i remember that in the past we had tons of issues with thread-local variables. They've been a pain in the ass on some platforms, and decided to not use them except for one case. Not sure if this is still the case, but if not, this needs to be updated:
https://github.com/bitcoin/bitcoin/blob/master/src/util/threadnames.cpp#L39
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2513309951)
i remember that in the past we had tons of issues with thread-local variables. They've been a pain in the ass on some platforms, and decided to not use them except for one case. Not sure if this is still the case, but if not, this needs to be updated:
https://github.com/bitcoin/bitcoin/blob/master/src/util/threadnames.cpp#L39
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3515590666)
I restored to the previous commit id `29ef3c62de`:
`29ef3c62de...d419fbc42b`: remove the functional test
`d419fbc42b..29ef3c62de`: restore the functional test
the fuzz test wasn't there in the first place.
I think it needs some more polishing and is not ready for a non-draft PR.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3515590666)
I restored to the previous commit id `29ef3c62de`:
`29ef3c62de...d419fbc42b`: remove the functional test
`d419fbc42b..29ef3c62de`: restore the functional test
the fuzz test wasn't there in the first place.
I think it needs some more polishing and is not ready for a non-draft PR.
💬 janb84 commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3515604722)
### My Guix Build Output
**Host architecture:** `aarch64`
**Commit:** `f06c6e189831`
```shell
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe72f917be109a68a2103e915183ed4fe01947 guix-build-f06c6e189831/outpu
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3515604722)
### My Guix Build Output
**Host architecture:** `aarch64`
**Commit:** `f06c6e189831`
```shell
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe72f917be109a68a2103e915183ed4fe01947 guix-build-f06c6e189831/outpu
...
🤔 janb84 reviewed a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3446920253)
Concept ACK f06c6e18983139dd63873b3537d2f87b8c6ec752
The build uses a lot more space 49,4 GIB ( 51 GiB (freespace before run - 1.6 GiB freespace after run) (will post proof later, have to run)
Good followup would be to adjust the guix build gate accordantly.
(will do some extra research on this to rule out something has changed on my guix vm)
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3446920253)
Concept ACK f06c6e18983139dd63873b3537d2f87b8c6ec752
The build uses a lot more space 49,4 GIB ( 51 GiB (freespace before run - 1.6 GiB freespace after run) (will post proof later, have to run)
Good followup would be to adjust the guix build gate accordantly.
(will do some extra research on this to rule out something has changed on my guix vm)
💬 laanwj commented on pull request "Changing the rpcbind argument being ignored to a pop up warning, inst…":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2513364532)
Do we want to change this one to a warning as well? It even say "WARNING".
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2513364532)
Do we want to change this one to a warning as well? It even say "WARNING".