💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718214663)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.
The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718214663)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.
The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than
...
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718237308)
That's strange, I don't see how a string literal would need a conversion when there's a `const char*` ctor? It seems to work fine for [`ConstevalStringLiteral`](https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/util/translation.h#L72) too, so pardon my insistence but I think perhaps this compiler error might be because of another issue? Would you mind trying this diff on 734ac5a9002493013c0f8afe763f751ac99f89c8?
<details>
<summary>git diff on 734ac5a900</su
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718237308)
That's strange, I don't see how a string literal would need a conversion when there's a `const char*` ctor? It seems to work fine for [`ConstevalStringLiteral`](https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/util/translation.h#L72) too, so pardon my insistence but I think perhaps this compiler error might be because of another issue? Would you mind trying this diff on 734ac5a9002493013c0f8afe763f751ac99f89c8?
<details>
<summary>git diff on 734ac5a900</su
...
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718238341)
I agree. Would either go all-in, or not do the rename at all.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718238341)
I agree. Would either go all-in, or not do the rename at all.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718218266)
nit: this is closely tied to `ParseHex_expected`, if you modify again, consider extracting all 5 usages next to it.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718218266)
nit: this is closely tied to `ParseHex_expected`, if you modify again, consider extracting all 5 usages next to it.
🤔 paplorinc reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240096017)
We're getting closer, I still hope we don't have to sacrifice some readability.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240096017)
We're getting closer, I still hope we don't have to sacrifice some readability.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718251992)
If we insist that this is an important check, it seems a bit jumpy to do the check at the beginning, after which we iterate over the elements and arrive exactly at that element - and just silently skip it.
Alternatively, we could check for `\0` when we get there, after we've processed the rest of the chars, i.e.
```C++
assert(!hex_str[i]);
return rv;
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718251992)
If we insist that this is an important check, it seems a bit jumpy to do the check at the beginning, after which we iterate over the elements and arrive exactly at that element - and just silently skip it.
Alternatively, we could check for `\0` when we get there, after we've processed the rest of the chars, i.e.
```C++
assert(!hex_str[i]);
return rv;
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718241896)
This was quite simple before, now it might be faster by a few nanos, but it's less readable.
Given that `HexStr` was already tested properly, could we maybe go the other way and convert `detsig` to hex string and compare against the hard coded value, i.e.
```suggestion
BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718241896)
This was quite simple before, now it might be faster by a few nanos, but it's less readable.
Given that `HexStr` was already tested properly, could we maybe go the other way and convert `detsig` to hex string and compare against the hard coded value, i.e.
```suggestion
BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718252997)
btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the `.run(` part, so I'd go with the cleanest code here, too.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718252997)
btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the `.run(` part, so I'd go with the cleanest code here, too.
💬 sipa commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2291069514)
@vasild Yes, but that gratuitously reveals to its peers that it is the transaction originator too (not really an issue in your private broadcast setting, but in nearly all others, this would be undesirable for that reason).
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2291069514)
@vasild Yes, but that gratuitously reveals to its peers that it is the transaction originator too (not really an issue in your private broadcast setting, but in nearly all others, this would be undesirable for that reason).
💬 twofaktor commented on issue "[FEATURE REQUEST] Enable new Tor PoW feature for automatic creation of Bitcoin Core onion hidden service":
(https://github.com/bitcoin/bitcoin/issues/28499#issuecomment-2291079391)
> @twofaktor if you'd like me to open a PR with this doc update, then let me know and I'd be happy to do that.
Hi, thanks for your dedication, IMO, if it is possible to add this secure protection feature in any of the variety of configuration cases (manually, without using the port control method), I think it should be added to the docs. If at some point the possibility of using it also using the port control arrives, add it when the time comes
(https://github.com/bitcoin/bitcoin/issues/28499#issuecomment-2291079391)
> @twofaktor if you'd like me to open a PR with this doc update, then let me know and I'd be happy to do that.
Hi, thanks for your dedication, IMO, if it is possible to add this secure protection feature in any of the variety of configuration cases (manually, without using the port control method), I think it should be added to the docs. If at some point the possibility of using it also using the port control arrives, add it when the time comes
💬 tdb3 commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2291090303)
> > Saw some `may be used uninitialized` warnings when building, e.g.
>
> What compiler? I don't see those warnings.
gcc/g++ (Debian 12.2.0-14) 12.2.0
Looks like it's on my end. Did a fresh clone and build and didn't see the same warnings.
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2291090303)
> > Saw some `may be used uninitialized` warnings when building, e.g.
>
> What compiler? I don't see those warnings.
gcc/g++ (Debian 12.2.0-14) 12.2.0
Looks like it's on my end. Did a fresh clone and build and didn't see the same warnings.
👍 tdb3 approved a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2240178506)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2240178506)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2240110993)
Concept ACK 1e37bcf9fc11562baaedea24685c31f60ef2de31
The current state of things with no/ad-hoc validation of command line args since #22766 in Nov 2021 is not a good place to be. There can be a tendency to neglect stringent argument parsing on larger projects as long as the happy path of correctly specified args still works.
Other more embryonic and radical initiatives exist to make args more strictly typed at compile time (#22978, and @ajtowns https://github.com/ajtowns/bitcoin/pull/8).
...
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2240110993)
Concept ACK 1e37bcf9fc11562baaedea24685c31f60ef2de31
The current state of things with no/ad-hoc validation of command line args since #22766 in Nov 2021 is not a good place to be. There can be a tendency to neglect stringent argument parsing on larger projects as long as the happy path of correctly specified args still works.
Other more embryonic and radical initiatives exist to make args more strictly typed at compile time (#22978, and @ajtowns https://github.com/ajtowns/bitcoin/pull/8).
...
💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1718228472)
The example for `ALLOW_INT | ALLOW_BOOL` in 6865a198f5db30bd494b3a2540f47ee728963908 is a bit worrying.
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2f2931cef1..e34bb35b7c 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3622,8 +3622,8 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
int prev_version = walletInstance->GetVersion();
if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))
{
-
...
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1718228472)
The example for `ALLOW_INT | ALLOW_BOOL` in 6865a198f5db30bd494b3a2540f47ee728963908 is a bit worrying.
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2f2931cef1..e34bb35b7c 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3622,8 +3622,8 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
int prev_version = walletInstance->GetVersion();
if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))
{
-
...
🤔 tdb3 reviewed a pull request: "addrman: change internal id counting to int64_t"
(https://github.com/bitcoin/bitcoin/pull/30568#pullrequestreview-2240216484)
Concept ACK.
Briefly reviewed, and will return to review in more detail. Love the approach to introduce `nid_type`. Makes things safer moving forward.
(https://github.com/bitcoin/bitcoin/pull/30568#pullrequestreview-2240216484)
Concept ACK.
Briefly reviewed, and will return to review in more detail. Love the approach to introduce `nid_type`. Makes things safer moving forward.
💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718299849)
This fails for me locally, because somehow I have 0000000000000000 as my xor key... but it fails consistently. Unsure if that's supposed to be possible?
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718299849)
This fails for me locally, because somehow I have 0000000000000000 as my xor key... but it fails consistently. Unsure if that's supposed to be possible?
👍 TheCharlatan approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2240220684)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2240220684)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?
💬 ismaelsadeeq commented on issue "wallet: setting changes are subject to race conditions":
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2291135117)
@wydengyre
Thanks for reporting the issue.
> It's likely possible to demonstrate this with a quick BASH or Python script that starts `bitcoind` and creates two wallets simultaneously.
I tried to recreate this in the functional test by spawning threads and calling `createwallet` simultaneously with `load_on_startup=True`.
<details>
<summary>See the diff</summary>
```diff
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
index 6feb00af8e1..2fed8
...
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2291135117)
@wydengyre
Thanks for reporting the issue.
> It's likely possible to demonstrate this with a quick BASH or Python script that starts `bitcoind` and creates two wallets simultaneously.
I tried to recreate this in the functional test by spawning threads and calling `createwallet` simultaneously with `load_on_startup=True`.
<details>
<summary>See the diff</summary>
```diff
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
index 6feb00af8e1..2fed8
...
📝 hodlinator opened a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660)
Without this PR, `BitcoinTestFramework.start_nodes()`, upon failing to establish RPC connections and calling `stop_nodes()`, would raise additional exceptions which prevented a clean shutdown.
Related: #30390
(https://github.com/bitcoin/bitcoin/pull/30660)
Without this PR, `BitcoinTestFramework.start_nodes()`, upon failing to establish RPC connections and calling `stop_nodes()`, would raise additional exceptions which prevented a clean shutdown.
Related: #30390
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291136157)
Encountered a case of #30390 here:
https://github.com/bitcoin/bitcoin/actions/runs/10380115902/job/28739417666?pr=30377
Output with only the test (no fix commit):
```
$ test/functional/feature_framework_rpc_failure.py
2024-08-15T07:30:20.427000Z TestFramework (INFO): PRNG seed is: 635212259632049987
2024-08-15T07:30:20.427000Z TestFramework (INFO): Initializing test directory /run/user/1001/bitcoin_func_test_ujsvbdnm
2024-08-15T07:30:20.428000Z TestFramework (INFO): Forcing us to timeou
...
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291136157)
Encountered a case of #30390 here:
https://github.com/bitcoin/bitcoin/actions/runs/10380115902/job/28739417666?pr=30377
Output with only the test (no fix commit):
```
$ test/functional/feature_framework_rpc_failure.py
2024-08-15T07:30:20.427000Z TestFramework (INFO): PRNG seed is: 635212259632049987
2024-08-15T07:30:20.427000Z TestFramework (INFO): Initializing test directory /run/user/1001/bitcoin_func_test_ujsvbdnm
2024-08-15T07:30:20.428000Z TestFramework (INFO): Forcing us to timeou
...