๐ฌ martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307235414)
@theStack I implemented the 3 suggestions. Let me know if you have further comments ๐
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307235414)
@theStack I implemented the 3 suggestions. Let me know if you have further comments ๐
๐ฌ virtu commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2307236913)
Only bumping this because there's a good chance this seed will serve zero addresses by release time.
Seeder been stuck [since January](https://21.ninja/dns-seeds/advertised-count/) and has deteriorated from 37 down to currently 5 addresses:
```bash
ยป for addr in seed.bitcoinstats.com x9.seed.bitcoinstats.com; do echo -n "unique addresses from $addr: " && host $addr | grep address | awk '{print $NF}' | sort -u | wc -l ; done
unique addresses from seed.bitcoinstats.com: 5
unique addresses
...
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2307236913)
Only bumping this because there's a good chance this seed will serve zero addresses by release time.
Seeder been stuck [since January](https://21.ninja/dns-seeds/advertised-count/) and has deteriorated from 37 down to currently 5 addresses:
```bash
ยป for addr in seed.bitcoinstats.com x9.seed.bitcoinstats.com; do echo -n "unique addresses from $addr: " && host $addr | grep address | awk '{print $NF}' | sort -u | wc -l ; done
unique addresses from seed.bitcoinstats.com: 5
unique addresses
...
๐ fanquake merged a pull request: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558)
(https://github.com/bitcoin/bitcoin/pull/30558)
๐ฌ furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1729102053)
> In commit "init: fix fatal error on '-wallet' negated option value" ([e56e2f5](https://github.com/bitcoin/bitcoin/commit/e56e2f51803b276945c74a479d30d884256d7ffc))
>
> If desired, you could include the value in the error message like `strprintf(_("Invalid -wallet value %s detected"), wallet.write())`
Sadly, that wouldn't help much and could end up confusing users. The error output for `-nowallet=not_a_boolean` would be `-wallet=true`, which does not correspond to what the user inputted.
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1729102053)
> In commit "init: fix fatal error on '-wallet' negated option value" ([e56e2f5](https://github.com/bitcoin/bitcoin/commit/e56e2f51803b276945c74a479d30d884256d7ffc))
>
> If desired, you could include the value in the error message like `strprintf(_("Invalid -wallet value %s detected"), wallet.write())`
Sadly, that wouldn't help much and could end up confusing users. The error output for `-nowallet=not_a_boolean` would be `-wallet=true`, which does not correspond to what the user inputted.
๐ฌ furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2307260467)
> Could update the description and note that this bug was caused by https://github.com/bitcoin/bitcoin/pull/22217.
Sure. Done!.
> I think this error could also be triggered by editing settings.json and adding a non-string wallet name.
Yes, nice. Added coverage for it too.
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2307260467)
> Could update the description and note that this bug was caused by https://github.com/bitcoin/bitcoin/pull/22217.
Sure. Done!.
> I think this error could also be triggered by editing settings.json and adding a non-string wallet name.
Yes, nice. Added coverage for it too.
๐ฌ stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729027931)
nit: could be made a bit more readable by using `auto` and implicitly constructing `Hex`:
<details>
<summary>git diff on df92661444</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 644c8ffc72..050aa2a5a9 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -154,15 +154,14 @@ BOOST_AUTO_TEST_CASE(parse_hex)
// Basic test vector
std::vector<unsigned char> expected(std::begin(HEX_PARSE_OUTPUT), std::end(HEX_PARSE_OUTP
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729027931)
nit: could be made a bit more readable by using `auto` and implicitly constructing `Hex`:
<details>
<summary>git diff on df92661444</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 644c8ffc72..050aa2a5a9 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -154,15 +154,14 @@ BOOST_AUTO_TEST_CASE(parse_hex)
// Basic test vector
std::vector<unsigned char> expected(std::begin(HEX_PARSE_OUTPUT), std::end(HEX_PARSE_OUTP
...
๐ฌ stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728787408)
Is there a specific reason to put `Hex` in the `detail` namespace? I think it could be useful to have `consteval` functions take a `Hex` parameter. For example, the `base_blob(string_view)` ctor can be quite elegantly deduplicated:
<details>
<summary>git diff on df92661444</summary>
```diff
diff --git a/src/uint256.h b/src/uint256.h
index 4124a34719..3616b0f9cb 100644
--- a/src/uint256.h
+++ b/src/uint256.h
@@ -16,6 +16,7 @@
#include <cstdint>
#include <cstring>
#include <optio
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728787408)
Is there a specific reason to put `Hex` in the `detail` namespace? I think it could be useful to have `consteval` functions take a `Hex` parameter. For example, the `base_blob(string_view)` ctor can be quite elegantly deduplicated:
<details>
<summary>git diff on df92661444</summary>
```diff
diff --git a/src/uint256.h b/src/uint256.h
index 4124a34719..3616b0f9cb 100644
--- a/src/uint256.h
+++ b/src/uint256.h
@@ -16,6 +16,7 @@
#include <cstdint>
#include <cstring>
#include <optio
...
๐ stickies-v approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2256959862)
ACK df92661444f46790b12d5061344d72106ef820d9 .
The new ""_hex literals approach makes for a much more ergonomic interface that is concise, clear and easy to use. Since the ""_hex operators are templated on the string literal, It does generate a lot of template instantiations, but since these are all consteval, I believe these should not end up in the binary, so that seems like a reasonable trade-off.
Left a few nits and a possible approach to further clean up the `base_blob::base_blob(str
...
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2256959862)
ACK df92661444f46790b12d5061344d72106ef820d9 .
The new ""_hex literals approach makes for a much more ergonomic interface that is concise, clear and easy to use. Since the ""_hex operators are templated on the string literal, It does generate a lot of template instantiations, but since these are all consteval, I believe these should not end up in the binary, so that seems like a reasonable trade-off.
Left a few nits and a possible approach to further clean up the `base_blob::base_blob(str
...
๐ฌ stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729071473)
nit in 6954b1f1143a73d4024f5564e287d8eeb34ff577: no more need for `MakeByteSpan`:
```suggestion
hw.write(G_uncompressed);
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729071473)
nit in 6954b1f1143a73d4024f5564e287d8eeb34ff577: no more need for `MakeByteSpan`:
```suggestion
hw.write(G_uncompressed);
```
๐ฌ stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729082336)
Since we're templating these on `Hex` instances, this string literal approach will produce quite a rather large number of template instantiations. I wonder if keeping these `consteval` would also better allow the compiler to make sure these instantiations don't end up in the binary?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729082336)
Since we're templating these on `Hex` instances, this string literal approach will produce quite a rather large number of template instantiations. I wonder if keeping these `consteval` would also better allow the compiler to make sure these instantiations don't end up in the binary?
๐ fanquake approved a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703#pullrequestreview-2257475032)
ACK fa5aeab3cb18405ecf8a1401d89539b924a618f6
(https://github.com/bitcoin/bitcoin/pull/30703#pullrequestreview-2257475032)
ACK fa5aeab3cb18405ecf8a1401d89539b924a618f6
๐ฌ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307287556)
> Find1P1CPackage doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit.
I think it might be more that `MempoolRejectedTx` doesn't remember if a tx failed for a different reason before considering an orphan?
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307287556)
> Find1P1CPackage doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit.
I think it might be more that `MempoolRejectedTx` doesn't remember if a tx failed for a different reason before considering an orphan?
๐ฌ l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307287794)
While I'm not a fan of mixed casing, I agree that it's probably outside the scope of current change.
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307287794)
While I'm not a fan of mixed casing, I agree that it's probably outside the scope of current change.
ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
๐ฌ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130184)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130184)
done
๐ฌ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130331)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130331)
done
๐ฌ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130506)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130506)
done
๐ฌ l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729138611)
> makes the definitions inconsistent and adds noise
I agree that it's likely just a documentation change for the reader (unsurprisingly, my trivial godbolt experiment also showed the same compiled output), but the methods *are* a bit different, isn't it useful signal if we mark them as such?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729138611)
> makes the definitions inconsistent and adds noise
I agree that it's likely just a documentation change for the reader (unsurprisingly, my trivial godbolt experiment also showed the same compiled output), but the methods *are* a bit different, isn't it useful signal if we mark them as such?
๐ฌ fanquake commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2307312714)
~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2307312714)
~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.
๐ TheCharlatan approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2257534072)
ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8
This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2257534072)
ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8
This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .
โ ๏ธ maflcko opened an issue: "Intermittent timeout on p2p_ibd_stalling.py"
(https://github.com/bitcoin/bitcoin/issues/30704)
On slow hardware (e.g. qemu s390x), the test may take long enough for a block timeout to hit.
Should be easy to fix with by using mocktime.
Test folder: https://drahtbot.space/temp_scratch/p2p_ibd_stalling_23.tar.zstd
Log:
```
node0 2024-08-22T06:47:16.815656Z [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 18f9c9ae8651e9aeb776ec51943c7fb4b813b1dedae262459d1915f69c345f52 from peer=0, disconnecting
node0 2024-08-22T06:47:16.815964Z [msghand] [net_proc
...
(https://github.com/bitcoin/bitcoin/issues/30704)
On slow hardware (e.g. qemu s390x), the test may take long enough for a block timeout to hit.
Should be easy to fix with by using mocktime.
Test folder: https://drahtbot.space/temp_scratch/p2p_ibd_stalling_23.tar.zstd
Log:
```
node0 2024-08-22T06:47:16.815656Z [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 18f9c9ae8651e9aeb776ec51943c7fb4b813b1dedae262459d1915f69c345f52 from peer=0, disconnecting
node0 2024-08-22T06:47:16.815964Z [msghand] [net_proc
...