💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1910687396)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1901095831
> Am I doing something wrong?
No, thanks for testing and this is a good catch. The commit message was describing what the change in behavior would have been if negating these options was allowed. But since these options aren't allowed to be negated their behavior isn't changing here. I updated the commit message and release notes and PR description to reflect this now. Thanks for catching it.
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1910687396)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1901095831
> Am I doing something wrong?
No, thanks for testing and this is a good catch. The commit message was describing what the change in behavior would have been if negating these options was allowed. But since these options aren't allowed to be negated their behavior isn't changing here. I updated the commit message and release notes and PR description to reflect this now. Thanks for catching it.
💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1910688540)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563
Thanks, I dropped the AssertionError now thanks to your other suggested simplification, and your observations above about assert statement usage and PYTHONOPTIMIZE are also interesting. IMO, it would be good to clean up the failing tests and maybe add a warning that some failures might not be detected if optimizations are turned on.
More broadly, I think there are common conventions for using `assert` statements that
...
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1910688540)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563
Thanks, I dropped the AssertionError now thanks to your other suggested simplification, and your observations above about assert statement usage and PYTHONOPTIMIZE are also interesting. IMO, it would be good to clean up the failing tests and maybe add a warning that some failures might not be detected if optimizations are turned on.
More broadly, I think there are common conventions for using `assert` statements that
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2583641138)
Still fails:
```
git clone https://github.com/ryanofsky/libmultiprocess.git
Cloning into 'libmultiprocess'...
cd libmultiprocess
git checkout pr/ucon
...
SUMMARY: UndefinedBehaviorSanitizer: dynamic-type-mismatch /usr/local/include/mp/proxy.h:95:45
```
https://github.com/bitcoin/bitcoin/actions/runs/12715313812/job/35447236983?pr=30975#step:7:5113
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2583641138)
Still fails:
```
git clone https://github.com/ryanofsky/libmultiprocess.git
Cloning into 'libmultiprocess'...
cd libmultiprocess
git checkout pr/ucon
...
SUMMARY: UndefinedBehaviorSanitizer: dynamic-type-mismatch /usr/local/include/mp/proxy.h:95:45
```
https://github.com/bitcoin/bitcoin/actions/runs/12715313812/job/35447236983?pr=30975#step:7:5113
💬 darosior commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2583651101)
> Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
As far as i remember we only ever store in the wallet descriptor produced by `ToString()`, which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a `pk()` expression this would be pretty minor.
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2583651101)
> Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
As far as i remember we only ever store in the wallet descriptor produced by `ToString()`, which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a `pk()` expression this would be pretty minor.
👍 dergoegge approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2543431435)
tACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2543431435)
tACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1911089511)
> But for this test in this PR it doesn't really matter I think
I think it does, previously we had three states (value, no value, exception) which kinda accidentally still made the test work, but in this PR there are only two (value, no value). So this test does not actually test what it claims it does, because if `QueryParameterExists` were to be changed to allow invalid characters, this test would still pass because there aren't any query parameters.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1911089511)
> But for this test in this PR it doesn't really matter I think
I think it does, previously we had three states (value, no value, exception) which kinda accidentally still made the test work, but in this PR there are only two (value, no value). So this test does not actually test what it claims it does, because if `QueryParameterExists` were to be changed to allow invalid characters, this test would still pass because there aren't any query parameters.
💬 furszy commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2583783893)
> > Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
>
> As far as i remember we only ever store in the wallet descriptor produced by `ToString()`, which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a `pk()` expression this would be pretty minor.
Was thinking about that a few days ago and star
...
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2583783893)
> > Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.
>
> As far as i remember we only ever store in the wallet descriptor produced by `ToString()`, which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a `pk()` expression this would be pretty minor.
Was thinking about that a few days ago and star
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1910681619)
nit: this could be `const` if you update `InitAndLoadChainstate()`'s `cache_sizes` parameter to `const`
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1910681619)
nit: this could be `const` if you update `InitAndLoadChainstate()`'s `cache_sizes` parameter to `const`
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1911227603)
Okay I'm sorry I can't seem to just let it go but here's another approach. It adds `SaturatingLeftShift` and `CheckedLeftShift()` `util/overflow.h` functionality (+tests, pinky promise), a `_MiB` string literal to allow remaining sizes to be converted to `size_t` bytes with concise notation and compile-time overflow guarantees (🤞). It also incorporates @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908969637) to just saturate -dbcache values instead of thro
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1911227603)
Okay I'm sorry I can't seem to just let it go but here's another approach. It adds `SaturatingLeftShift` and `CheckedLeftShift()` `util/overflow.h` functionality (+tests, pinky promise), a `_MiB` string literal to allow remaining sizes to be converted to `size_t` bytes with concise notation and compile-time overflow guarantees (🤞). It also incorporates @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908969637) to just saturate -dbcache values instead of thro
...
💬 pinheadmz commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2584010160)
I found another edge case in libevent regarding URI handling, it does not decode `%XX` characters before parsing.
This is in libevent's `evhttp_uri_parse()` which is called by Bitcoin's `GetQueryParameterFromUri()`
Example, the URI:
`/rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%25`
decodes to `/rest/endpoint/someresource.json?p1=v1&p2=100%`
It will be interpreted by libevent (and therefore bitcoin core, currently) as one long path with no query parameters.
Both Python's
...
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2584010160)
I found another edge case in libevent regarding URI handling, it does not decode `%XX` characters before parsing.
This is in libevent's `evhttp_uri_parse()` which is called by Bitcoin's `GetQueryParameterFromUri()`
Example, the URI:
`/rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%25`
decodes to `/rest/endpoint/someresource.json?p1=v1&p2=100%`
It will be interpreted by libevent (and therefore bitcoin core, currently) as one long path with no query parameters.
Both Python's
...
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1911267857)
> previously we had three states (value, no value, exception) which kinda accidentally still made the test work
I think the URI validation was performed before checking the params, so the exception was raised and caught by the test?
```
// URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried
uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2%";
BOOST_CHECK_EXCEPTION(GetQueryParameterFromUri(uri.c_str(), "p1"), std::runtime_error, HasReason
...
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1911267857)
> previously we had three states (value, no value, exception) which kinda accidentally still made the test work
I think the URI validation was performed before checking the params, so the exception was raised and caught by the test?
```
// URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried
uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2%";
BOOST_CHECK_EXCEPTION(GetQueryParameterFromUri(uri.c_str(), "p1"), std::runtime_error, HasReason
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1911315361)
> @hodlinator since you initially proposed the current solution, do you have any fresh opinions here?
When used with `size_t` constants, I get helpful GCC compiler errors when increasing the value of the constants (I guess this in part comes from C++ integer literals being signed 32-bit by default?):
```
/home/hodlinator/bitcoin/src/node/caches.cpp:23:53: error: narrowing conversion of ‘-176160768’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
23 | static constexpr size
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1911315361)
> @hodlinator since you initially proposed the current solution, do you have any fresh opinions here?
When used with `size_t` constants, I get helpful GCC compiler errors when increasing the value of the constants (I guess this in part comes from C++ integer literals being signed 32-bit by default?):
```
/home/hodlinator/bitcoin/src/node/caches.cpp:23:53: error: narrowing conversion of ‘-176160768’ from ‘int’ to ‘size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
23 | static constexpr size
...
💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2584171888)
> As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.
It means I can go back to the previous approach (simpler one) and only change the `ParsePubkeyInner`, there is no need to control it in the descriptor `Parse` anymore.
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2584171888)
> As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.
It means I can go back to the previous approach (simpler one) and only change the `ParsePubkeyInner`, there is no need to control it in the descriptor `Parse` anymore.
💬 darosior commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1911378183)
It seems much simpler and robust to just add the missing constructor than matching on available data and asserting?
```diff
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f8845..07214bdf2a4 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -527,27 +527,11 @@ struct Node {
{
// Use TreeEval() to avoid a stack-overflow due to recursion
auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
- NodeRe
...
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1911378183)
It seems much simpler and robust to just add the missing constructor than matching on available data and asserting?
```diff
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f8845..07214bdf2a4 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -527,27 +527,11 @@ struct Node {
{
// Use TreeEval() to avoid a stack-overflow due to recursion
auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
- NodeRe
...
💬 hodlinator commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1911403506)
That simplifies `Clone()` a lot, only thing I would add is make the new constructor `private` if we want to discourage outside uses.
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1911403506)
That simplifies `Clone()` a lot, only thing I would add is make the new constructor `private` if we want to discourage outside uses.
⚠️ jimhashhq opened an issue: "cmake: Compiling for test coverage (low-priority workaround exists)"
(https://github.com/bitcoin/bitcoin/issues/31638)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When following Compiling for Test Coverage instructions:
[doc/developer-notes-CompilingForTestCoverage](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage)
I encounter error:
```
CMake Error: File /home/alicebob/wkspc1/presets/bitcoin/cmake/cov_tool_wrapper.sh.in does not exist.
CMake Error at /home/alicebob/wkspc1/build/bitcoin/Covera
...
(https://github.com/bitcoin/bitcoin/issues/31638)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When following Compiling for Test Coverage instructions:
[doc/developer-notes-CompilingForTestCoverage](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-test-coverage)
I encounter error:
```
CMake Error: File /home/alicebob/wkspc1/presets/bitcoin/cmake/cov_tool_wrapper.sh.in does not exist.
CMake Error at /home/alicebob/wkspc1/build/bitcoin/Covera
...
💬 jimhashhq commented on issue "cmake: Compiling for test coverage (low-priority workaround exists)":
(https://github.com/bitcoin/bitcoin/issues/31638#issuecomment-2584643838)
Assuming this is not an execution or environment issue on my side, something like the following one-line addition to the top level project CMakeLists.txt resolves this (though maybe this can be simplified):
$ git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4b21646ca1..6753211ee5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -433,6 +433,7 @@ endif()
configure_file(cmake/script/Coverage.cmake Coverage.cmake USE_SOURCE_PERMISSIONS COPYONLY)
...
(https://github.com/bitcoin/bitcoin/issues/31638#issuecomment-2584643838)
Assuming this is not an execution or environment issue on my side, something like the following one-line addition to the top level project CMakeLists.txt resolves this (though maybe this can be simplified):
$ git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4b21646ca1..6753211ee5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -433,6 +433,7 @@ endif()
configure_file(cmake/script/Coverage.cmake Coverage.cmake USE_SOURCE_PERMISSIONS COPYONLY)
...
💬 triptrapz2 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/723c7526368badda15df8ac1ffc047a0ab2e384a#r151201565)
613btc
(https://github.com/bitcoin/bitcoin/commit/723c7526368badda15df8ac1ffc047a0ab2e384a#r151201565)
613btc
⚠️ triptrapz2 opened an issue: "bc1qp89r98q2d45gfgu0428p780ljea47ny2vm2yu3"
(https://github.com/bitcoin/bitcoin/issues/31639)
> 613btc send to wallet listed above
_Originally posted by @triptrapz2 in [723c752](https://github.com/bitcoin/bitcoin/commit/723c7526368badda15df8ac1ffc047a0ab2e384a#r151201565)_
(https://github.com/bitcoin/bitcoin/issues/31639)
> 613btc send to wallet listed above
_Originally posted by @triptrapz2 in [723c752](https://github.com/bitcoin/bitcoin/commit/723c7526368badda15df8ac1ffc047a0ab2e384a#r151201565)_
✅ pinheadmz closed an issue: "bc1qp89r98q2d45gfgu0428p780ljea47ny2vm2yu3"
(https://github.com/bitcoin/bitcoin/issues/31639)
(https://github.com/bitcoin/bitcoin/issues/31639)