💬 purpleKarrot commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318074319)
Here, you leave an empty `set_target_properties` call and remove it in the next commit. Just remove it directly.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318074319)
Here, you leave an empty `set_target_properties` call and remove it in the next commit. Just remove it directly.
💬 purpleKarrot commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318069759)
You modify the TODO in one commit and then delete it in another. You way want to rewrite the git history so that the todo is removed directly.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318069759)
You modify the TODO in one commit and then delete it in another. You way want to rewrite the git history so that the todo is removed directly.
💬 purpleKarrot commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318071503)
Same here. You modify the TODO in one commit and then delete it in the next one. Just delete it directly.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2318071503)
Same here. You modify the TODO in one commit and then delete it in the next one. Just delete it directly.
💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318090336)
> I think this should be a dependent option.
Why? Imo it's not unreasonable to want to build with only one of unit tests and functional tests, they don't depend on each other.
> Also, we should start prefixing all options to allow the project to be used as a subproject:
That sounds like a good idea, but pragmatically I think it makes sense to first open a separate issue, to discuss and get consensus on the goal and approach (e.g. prefix name), so I'm going to hold off on that here.
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318090336)
> I think this should be a dependent option.
Why? Imo it's not unreasonable to want to build with only one of unit tests and functional tests, they don't depend on each other.
> Also, we should start prefixing all options to allow the project to be used as a subproject:
That sounds like a good idea, but pragmatically I think it makes sense to first open a separate issue, to discuss and get consensus on the goal and approach (e.g. prefix name), so I'm going to hold off on that here.
💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248102026)
Very strange CI errors:
```
03:45:32.525] -- Configuring incomplete, errors occurred!
[03:45:32.543] ++ cmake -P /ci_container_base/ci/test/GetCMakeLogFiles.cmake
[03:45:32.586] + cat CMakeFiles/CMakeConfigureLog.yaml
[03:45:32.587] cat: CMakeFiles/CMakeConfigureLog.yaml: No such file or directory
[03:45:32.621] Command '['./ci/test/02_run_container.sh']' returned non-zero exit status 1.
```
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248102026)
Very strange CI errors:
```
03:45:32.525] -- Configuring incomplete, errors occurred!
[03:45:32.543] ++ cmake -P /ci_container_base/ci/test/GetCMakeLogFiles.cmake
[03:45:32.586] + cat CMakeFiles/CMakeConfigureLog.yaml
[03:45:32.587] cat: CMakeFiles/CMakeConfigureLog.yaml: No such file or directory
[03:45:32.621] Command '['./ci/test/02_run_container.sh']' returned non-zero exit status 1.
```
💬 purpleKarrot commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318148580)
> Imo it's not unreasonable to want to build with only one of unit tests and functional tests, they don't depend on each other.
They don't? `${BUILD_TESTS}` as the default value of the `BUILD_FUNCTIONAL_TESTS` option implies a dependency. If they are independent, maybe give them independent default values?
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318148580)
> Imo it's not unreasonable to want to build with only one of unit tests and functional tests, they don't depend on each other.
They don't? `${BUILD_TESTS}` as the default value of the `BUILD_FUNCTIONAL_TESTS` option implies a dependency. If they are independent, maybe give them independent default values?
💬 0xB10C commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3248190718)
> Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3248190718)
> Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248203281)
@cryptomeow
> Great! Regarding the Greek translation, I've gone one round and did some fixes based on the review and it certainly is helpful and provides actionable issues.
>
> One false positive group of issues I noticed is the disagreement on translating terms.
>
> For example one example is `Peers`, it seems the LLM insists on suggesting `Ομότιμοι` which is more about comparing people, while `Κόμβοι` is more commonly accepted in our case. Not sure how to address this so these issues
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248203281)
@cryptomeow
> Great! Regarding the Greek translation, I've gone one round and did some fixes based on the review and it certainly is helpful and provides actionable issues.
>
> One false positive group of issues I noticed is the disagreement on translating terms.
>
> For example one example is `Peers`, it seems the LLM insists on suggesting `Ομότιμοι` which is more about comparing people, while `Κόμβοι` is more commonly accepted in our case. Not sure how to address this so these issues
...
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248219824)
@l0rinc
> @cryptomeow you can check out this PR having the translations in an XML format and feed the whole file to an LLM. http://aistudio.google.com let's you work with multiple complete translations. You could ask it to extract the key/value pairs you want and feed _that_ to the other LLM of yours choice.
This is an interesting idea.
However, I don’t think it’s reasonable to ask volunteer coordinators to review the entire PR.
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248219824)
@l0rinc
> @cryptomeow you can check out this PR having the translations in an XML format and feed the whole file to an LLM. http://aistudio.google.com let's you work with multiple complete translations. You could ask it to extract the key/value pairs you want and feed _that_ to the other LLM of yours choice.
This is an interesting idea.
However, I don’t think it’s reasonable to ask volunteer coordinators to review the entire PR.
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318206490)
The example doesn't use `futures.wait`, but I guess `futures.wait` does not create copies of the futures, which would map differently as keys?
So I went ahead and implemented something like this.
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318206490)
The example doesn't use `futures.wait`, but I guess `futures.wait` does not create copies of the futures, which would map differently as keys?
So I went ahead and implemented something like this.
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248226196)
> ... but fwiw I found Transifex's Glossary very interesting to help have long-running async consistency on term translations, but I don't see any option to export them in order to possibly provide it as a context to the LLM.
I’m not aware of such an option either.
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248226196)
> ... but fwiw I found Transifex's Glossary very interesting to help have long-running async consistency on term translations, but I don't see any option to export them in order to possibly provide it as a context to the LLM.
I’m not aware of such an option either.
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318209717)
Yes, my understanding is that "sleep" and "wait" are words to describe a similar concept in this context. Leaving as-is for now.
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318209717)
Yes, my understanding is that "sleep" and "wait" are words to describe a similar concept in this context. Leaving as-is for now.
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318212456)
I prefer named fields with less floating symbols in the scope, so I'll leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2318212456)
I prefer named fields with less floating symbols in the scope, so I'll leave as-is for now.
💬 fanquake commented on pull request "rpc: deprecateboolverbose Boolean verbosity is deprecated":
(https://github.com/bitcoin/bitcoin/pull/33288#issuecomment-3248315087)
There's no need to open a new PR here, you can make these changes in #33214.
(https://github.com/bitcoin/bitcoin/pull/33288#issuecomment-3248315087)
There's no need to open a new PR here, you can make these changes in #33214.
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3248324405)
Force pushed with a tiny refactor turning a list into a dict. Should be easy to re-review.
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3248324405)
Force pushed with a tiny refactor turning a list into a dict. Should be easy to re-review.
✅ maflcko closed a pull request: "rpc: deprecateboolverbose Boolean verbosity is deprecated"
(https://github.com/bitcoin/bitcoin/pull/33288)
(https://github.com/bitcoin/bitcoin/pull/33288)
🤔 kannapoix reviewed a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3178686997)
ACK 69eeb11bc585e92a3b56811c3251f13dba05aa5f
I have run the all functional tests and reviewed the code.
I left a nit comment below.
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3178686997)
ACK 69eeb11bc585e92a3b56811c3251f13dba05aa5f
I have run the all functional tests and reviewed the code.
I left a nit comment below.
💬 kannapoix commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2317552470)
This error message might be confusing.
A message like “Private keys are disabled for this wallet” would be more intuitive, similar to what sendtoaddress returns.
https://github.com/bitcoin/bitcoin/blob/46369583f3a99d2b403349790f8b26dbc28de132/src/wallet/rpc/spend.cpp#L177-L179
I’m not suggesting to remove the current error.
It should still be raised when the wallet has private keys enabled but fails to determine the transaction size.
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2317552470)
This error message might be confusing.
A message like “Private keys are disabled for this wallet” would be more intuitive, similar to what sendtoaddress returns.
https://github.com/bitcoin/bitcoin/blob/46369583f3a99d2b403349790f8b26dbc28de132/src/wallet/rpc/spend.cpp#L177-L179
I’m not suggesting to remove the current error.
It should still be raised when the wallet has private keys enabled but fails to determine the transaction size.
💬 maflcko commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248398527)
> WalletDescriptor
This is fixed (removed) in https://github.com/bitcoin/bitcoin/pull/33082.
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3248398527)
> WalletDescriptor
This is fixed (removed) in https://github.com/bitcoin/bitcoin/pull/33082.
🤔 hodlinator reviewed a pull request: "index: store per-block transaction locations for efficient lookups"
(https://github.com/bitcoin/bitcoin/pull/32541#pullrequestreview-3179176604)
Reviewed 94389c28e1068ffcc116614d16ac3047eb3068e3
Measured a ~11% speed increase by enabling `LocationsIndex`, running on NVMe. Are you seeing greater improvements on other setups?
#### `-locationsindex=0`
```
₿ hey -n 100000 http://localhost:8332/rest/txfromblock/0000000000000000000083a0cff38278aae196d6d923a7e8ee7e5a0e371226fe-42.bin
Summary:
Total: 2.3504 secs
Slowest: 0.0193 secs
Fastest: 0.0001 secs
Average: 0.0012 secs
Requests/sec: 42545.2261
```
#### `-loca
...
(https://github.com/bitcoin/bitcoin/pull/32541#pullrequestreview-3179176604)
Reviewed 94389c28e1068ffcc116614d16ac3047eb3068e3
Measured a ~11% speed increase by enabling `LocationsIndex`, running on NVMe. Are you seeing greater improvements on other setups?
#### `-locationsindex=0`
```
₿ hey -n 100000 http://localhost:8332/rest/txfromblock/0000000000000000000083a0cff38278aae196d6d923a7e8ee7e5a0e371226fe-42.bin
Summary:
Total: 2.3504 secs
Slowest: 0.0193 secs
Fastest: 0.0001 secs
Average: 0.0012 secs
Requests/sec: 42545.2261
```
#### `-loca
...