💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071415173)
Usage looks correct according to documentation:
- `hAlgorithm`=`NULL`: required (see below)
- `pbBuffer`=`ent32`: "The address of a buffer that receives the random number. "
- `cbBuffer`=`NUM_OS_RANDOM_BYTES`: "The size, in bytes, of the pbBuffer buffer."
- `dwFlags`=`BCRYPT_USE_SYSTEM_PREFERRED_RNG`: "Use the system-preferred random number generator algorithm. The hAlgorithm parameter must be NULL."
Nit: Might want to use named arguments here.
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071415173)
Usage looks correct according to documentation:
- `hAlgorithm`=`NULL`: required (see below)
- `pbBuffer`=`ent32`: "The address of a buffer that receives the random number. "
- `cbBuffer`=`NUM_OS_RANDOM_BYTES`: "The size, in bytes, of the pbBuffer buffer."
- `dwFlags`=`BCRYPT_USE_SYSTEM_PREFERRED_RNG`: "Use the system-preferred random number generator algorithm. The hAlgorithm parameter must be NULL."
Nit: Might want to use named arguments here.
💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071420415)
We might need to update https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/symbol-check.py#L156
(likely add a DLL, possibly even remove one, though i doubt this was all we use from ADVAPI32).
The guix build will tell.
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071420415)
We might need to update https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/symbol-check.py#L156
(likely add a DLL, possibly even remove one, though i doubt this was all we use from ADVAPI32).
The guix build will tell.
💬 Sjors commented on issue "rfc: separate relay from mining policy":
(https://github.com/bitcoin/bitcoin/issues/32401#issuecomment-2846924406)
@ajtowns brought up one potential objection in the above thread, or at least something that increases implementation complexity:
> accepting txs into your mempool that you'll still relay but won't mine might block you from accepting other (conflicting) txs that you would mine, eg.
(https://github.com/bitcoin/bitcoin/issues/32401#issuecomment-2846924406)
@ajtowns brought up one potential objection in the above thread, or at least something that increases implementation complexity:
> accepting txs into your mempool that you'll still relay but won't mine might block you from accepting other (conflicting) txs that you would mine, eg.
💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846925861)
Fetched the updated Polish (pl) translation. Thanks to @jesterhodl !
@maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2833289993) has also been addressed.
I believe that any further translation improvements should be made through communication with the language coordinators, which I'm currently trying to arrange.
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846925861)
Fetched the updated Polish (pl) translation. Thanks to @jesterhodl !
@maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2833289993) has also been addressed.
I believe that any further translation improvements should be made through communication with the language coordinators, which I'm currently trying to arrange.
💬 0xB10C commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2846931412)
Trying it with `pfrom->GetId()` as the sole tracepoint argument produces the same failure. I don't think it's related to `ConnectionTypeAsString`.
It seems to me that all interface_usdt_*.py tests fail under valgrind?
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2846931412)
Trying it with `pfrom->GetId()` as the sole tracepoint argument produces the same failure. I don't think it's related to `ConnectionTypeAsString`.
It seems to me that all interface_usdt_*.py tests fail under valgrind?
👍 laanwj approved a pull request: "test: remove Boost SIGCHLD workaround."
(https://github.com/bitcoin/bitcoin/pull/32403#pullrequestreview-2811726146)
ACK 3add6ab9adcd722d57c6d488581358ae9b377f1a
(https://github.com/bitcoin/bitcoin/pull/32403#pullrequestreview-2811726146)
ACK 3add6ab9adcd722d57c6d488581358ae9b377f1a
💬 jesterhodl commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846937440)
Going forward:
- who do I contact if there's an issue with source labels?
- do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846937440)
Going forward:
- who do I contact if there's an issue with source labels?
- do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846937836)
cc @stickies-v @pablomartin4btc @johnny9 as regular reviewers of similar previous PRs.
cc @fanquake as the 29.1 release manager.
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846937836)
cc @stickies-v @pablomartin4btc @johnny9 as regular reviewers of similar previous PRs.
cc @fanquake as the 29.1 release manager.
💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846939416)
> Going forward:
>
> * who do I contact if there's an issue with source labels?
Feel free to ping me.
>
> * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
Mind clarifying please?
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2846939416)
> Going forward:
>
> * who do I contact if there's an issue with source labels?
Feel free to ping me.
>
> * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
Mind clarifying please?
💬 laanwj commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2846952751)
> This is not specific to CMake in general, nor to the Bitcoin Core build system.
Happy about that. i know it was something we explicitly considered and tested when going to CMake so hence my surprise.
In general, Windows users are the most likely to have spaces in any path they're allowed to write to. For example, from that iconv vcpkg issue:
> That would require me to reinstall my entire operating system (Windows 11 setup asks you for your name and then sets the user folder name to th
...
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2846952751)
> This is not specific to CMake in general, nor to the Bitcoin Core build system.
Happy about that. i know it was something we explicitly considered and tested when going to CMake so hence my surprise.
In general, Windows users are the most likely to have spaces in any path they're allowed to write to. For example, from that iconv vcpkg issue:
> That would require me to reinstall my entire operating system (Windows 11 setup asks you for your name and then sets the user folder name to th
...
💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071462315)
as expected:
```
unning symbol and dynamic library checks...
bcrypt.dll is not in ALLOWED_LIBRARIES!
```
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071462315)
as expected:
```
unning symbol and dynamic library checks...
bcrypt.dll is not in ALLOWED_LIBRARIES!
```
💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2846972614)
in the guix-built `bitcoind.exe` i can still see the `Crypt*` functions used (`objdump -p bitcoind.exe`):
```
DLL Name: ADVAPI32.dll
vma: Hint/Ord Member-Name Bound-To
dcf730 1194 CryptAcquireContextA
dcf748 1211 CryptGenRandom
dcf75a 1221 CryptReleaseContext
dcf770 1400 InitializeSecurityDescriptor
dcf790 1757 SetSecurityDescriptorDacl
```
So some other dependency is still using them.
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2846972614)
in the guix-built `bitcoind.exe` i can still see the `Crypt*` functions used (`objdump -p bitcoind.exe`):
```
DLL Name: ADVAPI32.dll
vma: Hint/Ord Member-Name Bound-To
dcf730 1194 CryptAcquireContextA
dcf748 1211 CryptGenRandom
dcf75a 1221 CryptReleaseContext
dcf770 1400 InitializeSecurityDescriptor
dcf790 1757 SetSecurityDescriptorDacl
```
So some other dependency is still using them.
💬 Brotcrunsher commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2846975819)
@laanwj I don't think that the time of the next release is necessarily the important thing here. This is documentation about how to build bitcoin core, which I assume is often done with the master of this repo.
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2846975819)
@laanwj I don't think that the time of the next release is necessarily the important thing here. This is documentation about how to build bitcoin core, which I assume is often done with the master of this repo.
💬 vasild commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071499260)
If the semantic of `bind_on_any` is to be changed then its comment deserves an update:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/net.h#L1072-L1074
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071499260)
If the semantic of `bind_on_any` is to be changed then its comment deserves an update:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/net.h#L1072-L1074
💬 vasild commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071496674)
I think this change to `GetListenPort()` will produce unwanted results if the configuration goes like:
`-bind=1.1.1.1:1111=onion -bind=2.2.2.2:2222`. In that case, I guess it should return `2222` like before this change, but after it, it would return `1111`.
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071496674)
I think this change to `GetListenPort()` will produce unwanted results if the configuration goes like:
`-bind=1.1.1.1:1111=onion -bind=2.2.2.2:2222`. In that case, I guess it should return `2222` like before this change, but after it, it would return `1111`.
💬 vasild commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071427640)
Right. This new code duplicates the code from `DefaultOnionServiceTarget()`:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/torcontrol.cpp#L726-L731
and the port logic from `init.cpp`:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/init.cpp#L1926
Which is problematic because if this logic has to be changed then it will have to be changed in more than one place.
Before this PR the outcome of `DefaultOnionSe
...
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r2071427640)
Right. This new code duplicates the code from `DefaultOnionServiceTarget()`:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/torcontrol.cpp#L726-L731
and the port logic from `init.cpp`:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/init.cpp#L1926
Which is problematic because if this logic has to be changed then it will have to be changed in more than one place.
Before this PR the outcome of `DefaultOnionSe
...
💬 fanquake commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847050625)
It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid? In either case, please adjust the hint to be explcit about spaces being unsupported, and remove vaugeries like "they have been known to cause build issues.". Either spaces are supported, in which case we can test the behaviour, or they are not supported.
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847050625)
It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid? In either case, please adjust the hint to be explcit about spaces being unsupported, and remove vaugeries like "they have been known to cause build issues.". Either spaces are supported, in which case we can test the behaviour, or they are not supported.
💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2071517155)
Nice!
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2071517155)
Nice!
💬 jesterhodl commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2847057846)
> > * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
>
>
>
> Mind clarifying please?
Sure. I translate lots of apps
on Transifex, Weblate, etc.
I routinely revisit to check for new things to update or correct existing translations based on my experience or feedback.
So I update the transifex translations.
When I do this, is it sufficient and github pulls the .ts files automatically, or would it be wise to ping someone?
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2847057846)
> > * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
>
>
>
> Mind clarifying please?
Sure. I translate lots of apps
on Transifex, Weblate, etc.
I routinely revisit to check for new things to update or correct existing translations based on my experience or feedback.
So I update the transifex translations.
When I do this, is it sufficient and github pulls the .ts files automatically, or would it be wise to ping someone?
💬 hebasto commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847086592)
> It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid?
`libiconv` is listed as a direct dependency of the `qt` package (see the output of `vcpkg depend-info qt`).
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847086592)
> It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid?
`libiconv` is listed as a direct dependency of the `qt` package (see the output of `vcpkg depend-info qt`).