💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580846838)
From what I could tell, we don’t expose this anywhere. I’m in favor of making this the default since we only ever really talking about outpoints in terms of their serialised encoding. Gonna give it some more thought, but I’m considering opening this commit as its own PR. It seems preferable to me to have one way of sorting outpoints vs creating a special case for silent payments, especially when there doesn’t seem to be any other “sort outpoints” use case (e.g. none of the tests failed when I ma
...
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580846838)
From what I could tell, we don’t expose this anywhere. I’m in favor of making this the default since we only ever really talking about outpoints in terms of their serialised encoding. Gonna give it some more thought, but I’m considering opening this commit as its own PR. It seems preferable to me to have one way of sorting outpoints vs creating a special case for silent payments, especially when there doesn’t seem to be any other “sort outpoints” use case (e.g. none of the tests failed when I ma
...
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580850619)
But still need to verify re: displayed in RPCs. At that point tho, we could just sort by `vout` and ignore the txid in the sort
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580850619)
But still need to verify re: displayed in RPCs. At that point tho, we could just sort by `vout` and ignore the txid in the sort
💬 naiyoma commented on pull request "net: update comment for service bit support info for seed.bitcoin.sipa.be":
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2079140202)
> I also think it would be good to get this out of chainparams, bitcoind only ever uses "x9" anyway as far as I can see, so I'm not sure why the status of other combinations needs to be tracked here. Maybe adding the info in the first place was meant as a service to other software that would actually use those other combinations?
I'm working on this [script](https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/162) to filter seeds, I think it's okay to delete all the combinations co
...
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2079140202)
> I also think it would be good to get this out of chainparams, bitcoind only ever uses "x9" anyway as far as I can see, so I'm not sure why the status of other combinations needs to be tracked here. Maybe adding the info in the first place was meant as a service to other software that would actually use those other combinations?
I'm working on this [script](https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/162) to filter seeds, I think it's okay to delete all the combinations co
...
👍 laanwj approved a pull request: "guix: remove bzip2 from deps"
(https://github.com/bitcoin/bitcoin/pull/29895#pullrequestreview-2024816991)
Compared the binaries. `bitcoin-qt` has three differences:
- `_Z17FormatFullVersionB5cxx11v`
- version string in `.rodata` section
- checksum in `.debug_info` section
This is fully expected. ACK b8e084b9781eaa4d624a3c1d58b39c07005a0e13
(https://github.com/bitcoin/bitcoin/pull/29895#pullrequestreview-2024816991)
Compared the binaries. `bitcoin-qt` has three differences:
- `_Z17FormatFullVersionB5cxx11v`
- version string in `.rodata` section
- checksum in `.debug_info` section
This is fully expected. ACK b8e084b9781eaa4d624a3c1d58b39c07005a0e13
💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580901500)
Separate PR sounds good to get more eyes on it. Meanwhile this is fine a temporary fix for this PR.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1580901500)
Separate PR sounds good to get more eyes on it. Meanwhile this is fine a temporary fix for this PR.
💬 nanlour commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079233223)
> Not going to look at it right now, but I suspect this fix just creates a different race instead.
> Looks like furszy already got this in https://github.com/bitcoin/bitcoin/pull/29867
I think the race condition fixed in #29867 was not introduced by my change; it was created by #28955, although my change makes the race condition window bigger.
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079233223)
> Not going to look at it right now, but I suspect this fix just creates a different race instead.
> Looks like furszy already got this in https://github.com/bitcoin/bitcoin/pull/29867
I think the race condition fixed in #29867 was not introduced by my change; it was created by #28955, although my change makes the race condition window bigger.
💬 laanwj commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1580927959)
ACK on improving the syntax, but it seems a matter of time before the compiler again becomes smart enough to see this variable is unused?
Mayb eadd
```
(void)unix;
```
to the `!HAVE_SOCKADDR_UN` path just in case?
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1580927959)
ACK on improving the syntax, but it seems a matter of time before the compiler again becomes smart enough to see this variable is unused?
Mayb eadd
```
(void)unix;
```
to the `!HAVE_SOCKADDR_UN` path just in case?
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580933666)
We're breaking the format anyway with #29612 so might as well introduce other breaking changes now.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580933666)
We're breaking the format anyway with #29612 so might as well introduce other breaking changes now.
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580936006)
The window can contain the snapshot height? So then it should really check that _all_ recount values exist.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580936006)
The window can contain the snapshot height? So then it should really check that _all_ recount values exist.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079270744)
Indexes match! If there's other implementations out there, we should do the same comparison.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079270744)
Indexes match! If there's other implementations out there, we should do the same comparison.
📝 iw4p opened a pull request: "refactor: convert string formatting to F-strings"
(https://github.com/bitcoin/bitcoin/pull/29969)
All instances of string formatting have been converted to F-strings for increased readability and performance. Before that some of strings use `.format` and others use f-strings.
(https://github.com/bitcoin/bitcoin/pull/29969)
All instances of string formatting have been converted to F-strings for increased readability and performance. Before that some of strings use `.format` and others use f-strings.
📝 iw4p opened a pull request: "refactor: switch from curl to requests for HTTP requests"
(https://github.com/bitcoin/bitcoin/pull/29970)
Switched from using subprocess to make HTTP requests (curl) to using the Python built-in requests library, improving cleanliness and maintainability.
(https://github.com/bitcoin/bitcoin/pull/29970)
Switched from using subprocess to make HTTP requests (curl) to using the Python built-in requests library, improving cleanliness and maintainability.
📝 iw4p opened a pull request: "refactor: refactored platform assignment into get_platform function"
(https://github.com/bitcoin/bitcoin/pull/29971)
Improved code readability and maintainability by moving platform identifiers into a dictionary.
(https://github.com/bitcoin/bitcoin/pull/29971)
Improved code readability and maintainability by moving platform identifiers into a dictionary.
💬 ryanofsky commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079292470)
> Until #28955, `cs_main` was held across all this to prevent a race.
The race existed before #28955 because even before #28955, only the `BaseIndex::Sync` thread was locking `cs_main`. The `BaseIndex::BlockConnected` notification thread was never locking `cs_main`. So setting `m_synced = true;` before calling `Commit();` was always unsafe, even with `cs_main` held because the `BlockConnected` thread could run in parallel and both threads could try to update index state at the same time. #289
...
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079292470)
> Until #28955, `cs_main` was held across all this to prevent a race.
The race existed before #28955 because even before #28955, only the `BaseIndex::Sync` thread was locking `cs_main`. The `BaseIndex::BlockConnected` notification thread was never locking `cs_main`. So setting `m_synced = true;` before calling `Commit();` was always unsafe, even with `cs_main` held because the `BlockConnected` thread could run in parallel and both threads could try to update index state at the same time. #289
...
💬 maflcko commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580961406)
This is not a refactor. It is a behavior change when an error occurs, for example a network error, or a file error
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580961406)
This is not a refactor. It is a behavior change when an error occurs, for example a network error, or a file error
💬 maflcko commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#discussion_r1580962931)
Not sure. Now there are two functions and more lines of code, so this is harder to read and to maintain
(https://github.com/bitcoin/bitcoin/pull/29971#discussion_r1580962931)
Not sure. Now there are two functions and more lines of code, so this is harder to read and to maintain
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580966108)
> The window can contain the snapshot height? If so then it should really check that _all_ `txcount` values exist.
Why? Can you explain this a bit more? The result is not influenced by values that are not used in the calculation. For example, if the windows is [a, x, y, z, b] and you calculate `b - a`, then the values of x, y, z are irrelevant.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580966108)
> The window can contain the snapshot height? If so then it should really check that _all_ `txcount` values exist.
Why? Can you explain this a bit more? The result is not influenced by values that are not used in the calculation. For example, if the windows is [a, x, y, z, b] and you calculate `b - a`, then the values of x, y, z are irrelevant.
💬 laanwj commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079302092)
@vasild Do you know if C++ thread-local storage is still broken on FreeBSD?
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079302092)
@vasild Do you know if C++ thread-local storage is still broken on FreeBSD?
💬 ryanofsky commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2079303078)
Yes in terms of backports, it could make sense to backport the #29776 fix to releases before #28955, since the race fixed by #29776 where two threads are trying to update index state at the same time at the end of a sync could happen before #28955, although it was probably much less likely to happen before #28955.
It would only make sense to backport this PR to releases after #28955, though, since this bug, which could cause blocks to be missing from the index, was introduced in #28955.
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2079303078)
Yes in terms of backports, it could make sense to backport the #29776 fix to releases before #28955, since the race fixed by #29776 where two threads are trying to update index state at the same time at the end of a sync could happen before #28955, although it was probably much less likely to happen before #28955.
It would only make sense to backport this PR to releases after #28955, though, since this bug, which could cause blocks to be missing from the index, was introduced in #28955.
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580968591)
> We're breaking the format anyway with #29612 so might as well introduce other breaking changes now.
Not sure why that pull request would be relevant to the changes here. The two RPCs are completely independent and the behavior or interface of one should not interact with the other.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580968591)
> We're breaking the format anyway with #29612 so might as well introduce other breaking changes now.
Not sure why that pull request would be relevant to the changes here. The two RPCs are completely independent and the behavior or interface of one should not interact with the other.