💬 le0nAg commented on issue "Add `sat_to_btc()` and conversely `btc_to_sat()` util functions in functional tests":
(https://github.com/bitcoin/bitcoin/issues/31345#issuecomment-2516322672)
Ok, thanks! I took yesterday and today to understand at which point is the PR.
Tomorrow I will fork it
(https://github.com/bitcoin/bitcoin/issues/31345#issuecomment-2516322672)
Ok, thanks! I took yesterday and today to understand at which point is the PR.
Tomorrow I will fork it
👍 maflcko approved a pull request: "test: Prove+document ConstevalFormatString/tinyformat parity"
(https://github.com/bitcoin/bitcoin/pull/30933#pullrequestreview-2477481144)
lgtm, but it would be better to merge the conflicting non-draft pull with two acks first. This will also avoid having to touch some lines twice, which are touched here.
(https://github.com/bitcoin/bitcoin/pull/30933#pullrequestreview-2477481144)
lgtm, but it would be better to merge the conflicting non-draft pull with two acks first. This will also avoid having to touch some lines twice, which are touched here.
💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1868849318)
Could do a third listwalletdir without the symlink to check that the test is working by checking for absence of `error scanning`?
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1868849318)
Could do a third listwalletdir without the symlink to check that the test is working by checking for absence of `error scanning`?
💬 maflcko commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516416418)
I had the same thought. Shouldn't a venv "just work" out of the box, because they set the `PATH` appropriately?
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516416418)
I had the same thought. Shouldn't a venv "just work" out of the box, because they set the `PATH` appropriately?
💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2516421998)
For reference the CI failure is:
```
subprocess.CalledProcessError: Command '['C:\\hostedtoolcache\\windows\\Python\\3.12.7\\x64\\python.exe', 'D:/a/bitcoin/bitcoin\\contrib\\signet\\miner', "--cli='D:\\a\\bitcoin\\bitcoin\\build\\src\\Release\\bitcoin-cli.exe' -nonamed '-datadir=D:\\a\\_temp\\test_runner_₿_🏃_20241203_223434\\tool_signet_miner_211\\node0'", 'generate', '--address=tb1q2ndfasp67k5wp30fkt63tw9gf465lcjf7rm5fc', "--grind-cmd='D:\\a\\bitcoin\\bi
...
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2516421998)
For reference the CI failure is:
```
subprocess.CalledProcessError: Command '['C:\\hostedtoolcache\\windows\\Python\\3.12.7\\x64\\python.exe', 'D:/a/bitcoin/bitcoin\\contrib\\signet\\miner', "--cli='D:\\a\\bitcoin\\bitcoin\\build\\src\\Release\\bitcoin-cli.exe' -nonamed '-datadir=D:\\a\\_temp\\test_runner_₿_🏃_20241203_223434\\tool_signet_miner_211\\node0'", 'generate', '--address=tb1q2ndfasp67k5wp30fkt63tw9gf465lcjf7rm5fc', "--grind-cmd='D:\\a\\bitcoin\\bi
...
💬 summraznboi commented on issue "Cluster mempool tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30289#issuecomment-2516463194)
Wow, no ancestor/descendant limit would be great for unblocking UX hurdles in creating transactions! (Commenting to watch, also curious how someone like myself could help out here).
(https://github.com/bitcoin/bitcoin/issues/30289#issuecomment-2516463194)
Wow, no ancestor/descendant limit would be great for unblocking UX hurdles in creating transactions! (Commenting to watch, also curious how someone like myself could help out here).
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2516465730)
Rebased 8598bc9e5d3fb7ebc08cf0c6422b3e44c56230d6 -> 6090df267dfece6192b567fed6582445aa811e7f ([kernelApi_8](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_8) -> [kernelApi_9](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_8..kernelApi_9))
* Alligned process block pre-checks through https://github.com/bitcoin/bitcoin/pull/31175
* Clamp the work threads number so we properly handle the value range through
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2516465730)
Rebased 8598bc9e5d3fb7ebc08cf0c6422b3e44c56230d6 -> 6090df267dfece6192b567fed6582445aa811e7f ([kernelApi_8](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_8) -> [kernelApi_9](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_8..kernelApi_9))
* Alligned process block pre-checks through https://github.com/bitcoin/bitcoin/pull/31175
* Clamp the work threads number so we properly handle the value range through
...
💬 carnhofdaki commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2516525566)
Concept NACK.
I use a simple script setting datadir according to the current working directory. If the change would be backward-compatible I don't care. But running multiple custom signets and never needed to make a mess. The proposed naming convention (`signet_XXXXXXX`) seems like a mess to me.
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2516525566)
Concept NACK.
I use a simple script setting datadir according to the current working directory. If the change would be backward-compatible I don't care. But running multiple custom signets and never needed to make a mess. The proposed naming convention (`signet_XXXXXXX`) seems like a mess to me.
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869004667)
I was wondering how it will behave with `%c` which would add a `\0` which could end the const char* prematurely, but since we have a string it seems to work correctly.
Are there any checks we could do with the result here? Or make `TfmFormatZeroes` void if we don't need the actual result. Maybe something like this:
```suggestion
auto result{TfmFormatZeroes<NumArgs>(fmt.fmt)};
BOOST_CHECK_EQUAL(result.empty(), std::string_view{fmt.fmt}.empty());
```
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869004667)
I was wondering how it will behave with `%c` which would add a `\0` which could end the const char* prematurely, but since we have a string it seems to work correctly.
Are there any checks we could do with the result here? Or make `TfmFormatZeroes` void if we don't need the actual result. Maybe something like this:
```suggestion
auto result{TfmFormatZeroes<NumArgs>(fmt.fmt)};
BOOST_CHECK_EQUAL(result.empty(), std::string_view{fmt.fmt}.empty());
```
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869005752)
Is it coincidental that every single format type accepts `0` as a valid substitution?
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869005752)
Is it coincidental that every single format type accepts `0` as a valid substitution?
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1868990468)
👍 for checking `%n` as well!
nit: we might inline `0` here:
```suggestion
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, 0), tfm::format_error,
```
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1868990468)
👍 for checking `%n` as well!
nit: we might inline `0` here:
```suggestion
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, 0), tfm::format_error,
```
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1869077815)
Thanks! Done.
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1869077815)
Thanks! Done.
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547)
> Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
Apparently not. Here's another case where CMake just picks some other Python:
```bash
# python --version
Python 3.13.1
# python3 --version
Python 3.13.1
# which python
/Users/xxx/.pyenv/shims/python
# cmake -B build
<snip>
-- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpr
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547)
> Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
Apparently not. Here's another case where CMake just picks some other Python:
```bash
# python --version
Python 3.13.1
# python3 --version
Python 3.13.1
# which python
/Users/xxx/.pyenv/shims/python
# cmake -B build
<snip>
-- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpr
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869162815)
Even better! Added `-U`.
With `-U` is does not matter anymore, but I think it was ok before as well, see my comment [below](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867509291).
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869162815)
Even better! Added `-U`.
With `-U` is does not matter anymore, but I think it was ok before as well, see my comment [below](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867509291).
📝 maflcko opened a pull request: " doc: Fix incorrect send RPC docs "
(https://github.com/bitcoin/bitcoin/pull/31416)
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue and also creates a special dedicated type for an import timestamp.
(https://github.com/bitcoin/bitcoin/pull/31416)
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue and also creates a special dedicated type for an import timestamp.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869187689)
To 1. flush the file and 2. so that the next set of tests can start fresh without leftover `tcpdump` processes. Now 1. is not needed because of `-U`, but I will leave it for 2.
In my experiments `SIGTERM` would cause `tcpdump` to exit gracefully - flush the file and then exit. `SIGKILL` would result in unflushed file.
Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869187689)
To 1. flush the file and 2. so that the next set of tests can start fresh without leftover `tcpdump` processes. Now 1. is not needed because of `-U`, but I will leave it for 2.
In my experiments `SIGTERM` would cause `tcpdump` to exit gracefully - flush the file and then exit. `SIGKILL` would result in unflushed file.
Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516927066)
> > Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
>
> Apparently not. Here's another case where CMake just picks some other Python:
>
> ```shell
> # python --version
> Python 3.13.1
> # python3 --version
> Python 3.13.1
> # which python
> /Users/xxx/.pyenv/shims/python
> # cmake -B build
> <snip>
> -- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516927066)
> > Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
>
> Apparently not. Here's another case where CMake just picks some other Python:
>
> ```shell
> # python --version
> Python 3.13.1
> # python3 --version
> Python 3.13.1
> # which python
> /Users/xxx/.pyenv/shims/python
> # cmake -B build
> <snip>
> -- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869206235)
> Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
That is interesting, because all tasks should be running on workers that are set up identically.
> tests can start fresh without leftover `tcpdump` processes
My thinking was to just have a single process, which is started once and never killed. This should remove the need to kill it, and also the need (and code) to start it every time.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869206235)
> Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
That is interesting, because all tasks should be running on workers that are set up identically.
> tests can start fresh without leftover `tcpdump` processes
My thinking was to just have a single process, which is started once and never killed. This should remove the need to kill it, and also the need (and code) to start it every time.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837)
This is also ignoring the return code? So it could also lead to CI silently passing on all tasks, when it should not.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837)
This is also ignoring the return code? So it could also lead to CI silently passing on all tasks, when it should not.
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.