📝 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.
💬 maflcko commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079308571)
> FreeBSD
I am not familiar with the *BSD, but is there indication that OpenBSD or NetBSD are unaffected? What are the steps to test this?
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079308571)
> FreeBSD
I am not familiar with the *BSD, but is there indication that OpenBSD or NetBSD are unaffected? What are the steps to test this?
💬 laanwj commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580971663)
If you're going to read the entire data into memory, you might as well do the hashing at the same time? Currently, it reads back the same `tarball` it wrote and passes it to the hasher, which seems kind of a waste.
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580971663)
If you're going to read the entire data into memory, you might as well do the hashing at the same time? Currently, it reads back the same `tarball` it wrote and passes it to the hasher, which seems kind of a waste.
💬 iw4p commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580972221)
You're right about behavior change. I am still not sure about using `script` or `util` tag.
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1580972221)
You're right about behavior change. I am still not sure about using `script` or `util` tag.
💬 laanwj commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1580975109)
Now you're assigning a value that isn't used, i think that will generate a warning too. Doesn't the suggested `(void)unix;` work?
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1580975109)
Now you're assigning a value that isn't used, i think that will generate a warning too. Doesn't the suggested `(void)unix;` work?
💬 laanwj commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079320031)
i understand you're only trying to help, but please don't flood the project with small Python script refactorings. We're severely bottlenecked on review, and tend to have a philosophy to not fix what isn't broken.
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079320031)
i understand you're only trying to help, but please don't flood the project with small Python script refactorings. We're severely bottlenecked on review, and tend to have a philosophy to not fix what isn't broken.
💬 theuni commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079322949)
> However, switching the CC variable context from Makefile to the shell environment breaks expectations
Arguably that's because this wasn't intended to be supported :)
I suppose this fix is reasonable, though supporting env vars like this seems quite brittle.
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2079322949)
> However, switching the CC variable context from Makefile to the shell environment breaks expectations
Arguably that's because this wasn't intended to be supported :)
I suppose this fix is reasonable, though supporting env vars like this seems quite brittle.
💬 iw4p commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079331040)
I understood but if the philosophy is to not fix what isn't broken, so what "refactor" (for structural changes that do not change behavior) means? I am really trying to start reading the code from easy parts to contribute and refactor codes and update them. Some codes are for more than 4 years ago which are using subprocess for a single curl, which is not optimized when python's providing requests built-in library.
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079331040)
I understood but if the philosophy is to not fix what isn't broken, so what "refactor" (for structural changes that do not change behavior) means? I am really trying to start reading the code from easy parts to contribute and refactor codes and update them. Some codes are for more than 4 years ago which are using subprocess for a single curl, which is not optimized when python's providing requests built-in library.
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580972472)
```
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1707:34: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<unsigned int, true, true>::<unnamed>)).std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>::_M_value’ may be used uninitialized
...
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1580972472)
```
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1707:34: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<unsigned int, true, true>::<unnamed>)).std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>::_M_value’ may be used uninitialized
...
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581000551)
Both are listed as critical for mainnet release in #29616. If one needs a breaking change, it's fine for the other to also make one.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581000551)
Both are listed as critical for mainnet release in #29616. If one needs a breaking change, it's fine for the other to also make one.
💬 laanwj commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079350937)
Sure, but for example: if you're going to do things like optimization, consider if speed is important in the process at that point (in many cases it's not, that's why the tool is in Python and not C++). If it is important, we'll also need to see benchmarks that the speed is noticibly faster.
If you've been involved for a while and know where the pain points are, this is easier to know. For new contributors I wouldn't really recommend doing refactors at all.
If you're looking for something
...
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079350937)
Sure, but for example: if you're going to do things like optimization, consider if speed is important in the process at that point (in many cases it's not, that's why the tool is in Python and not C++). If it is important, we'll also need to see benchmarks that the speed is noticibly faster.
If you've been involved for a while and know where the pain points are, this is easier to know. For new contributors I wouldn't really recommend doing refactors at all.
If you're looking for something
...
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581006151)
Let's say we have blocks [1,2,3,4,5,6,7,8,9] and block 6 is the snapshot. The background sync just processed block 3, foreground sync is past 9. You ask for the window 1 - 7. In that case `nChainTx` for (1) and (7) are set, but's not set for (4) and (5).
However as you say, this not a problem when subtracting `nChainTx` of block (1) from `nChainTx` of block (7).
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581006151)
Let's say we have blocks [1,2,3,4,5,6,7,8,9] and block 6 is the snapshot. The background sync just processed block 3, foreground sync is past 9. You ask for the window 1 - 7. In that case `nChainTx` for (1) and (7) are set, but's not set for (4) and (5).
However as you say, this not a problem when subtracting `nChainTx` of block (1) from `nChainTx` of block (7).