💬 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).
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#issuecomment-2079355204)
Addressed @theStack and @sr-gi comments. Thanks!
(https://github.com/bitcoin/bitcoin/pull/28970#issuecomment-2079355204)
Addressed @theStack and @sr-gi comments. Thanks!
💬 fanquake commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079363170)
Backported for 27.x in #29888.
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2079363170)
Backported for 27.x in #29888.
💬 fanquake 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-2079364397)
I've pulled #29776 in for 27.x. It could go into 26.x as well cc @glozow.
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2079364397)
I've pulled #29776 in for 27.x. It could go into 26.x as well cc @glozow.
💬 iw4p commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079364688)
I appreciate your response and explanation. I'll take a quick look into it!
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079364688)
I appreciate your response and explanation. I'll take a quick look into it!
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581015988)
> 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.
Not sure. That doesn't mean that https://github.com/bitcoin/bitcoin/pull/29612 will be merged in its current form. One could come up with a non-breaking version of 29612, for example.
I think the pull requests should be evaluated independently.
In any case, when evaluating whether *this pull request* is a breaking change, I don't think it can affect any
...
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581015988)
> 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.
Not sure. That doesn't mean that https://github.com/bitcoin/bitcoin/pull/29612 will be merged in its current form. One could come up with a non-breaking version of 29612, for example.
I think the pull requests should be evaluated independently.
In any case, when evaluating whether *this pull request* is a breaking change, I don't think it can affect any
...
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581021830)
> Which seems spurious, but it goes away with e.g.:
Not sure about changing the code for a broken compiler warning. I am sure this warning already exists in other parts of the code, so if changing the code is a desire, then it should be done for the other places as well (and CI could check against it). However, my recommendation would be to report this to upstream with a minimal reproducer, if it still happens with g++14.0.1-beta
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581021830)
> Which seems spurious, but it goes away with e.g.:
Not sure about changing the code for a broken compiler warning. I am sure this warning already exists in other parts of the code, so if changing the code is a desire, then it should be done for the other places as well (and CI could check against it). However, my recommendation would be to report this to upstream with a minimal reproducer, if it still happens with g++14.0.1-beta
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581023235)
> However as you say, this not a problem when subtracting `nChainTx` of block (1) from `nChainTx` of block (7).
Ok, not sure what to do here then. I'll probably resolve the thread for now.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581023235)
> However as you say, this not a problem when subtracting `nChainTx` of block (1) from `nChainTx` of block (7).
Ok, not sure what to do here then. I'll probably resolve the thread for now.
💬 maflcko commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079383903)
Also:
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.
If you need
...
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079383903)
Also:
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.
If you need
...
💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2079388456)
Posting my response to part of [your comment on #29906](https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007) here, since `Update()` is no longer relevant to that PR:
> Examples would be the [BlockManager::SaveBlockToDisk](https://github.com/ryanofsky/bitcoin/blob/4d2c9de24916f8d69514ea7c7251136e2762fa5c/src/node/blockstorage.cpp#L1167-L1195) function from https://github.com/bitcoin/bitcoin/pull/29700
Had just a quick look, but this looks like how I'd expect `Update()` to
...
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2079388456)
Posting my response to part of [your comment on #29906](https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007) here, since `Update()` is no longer relevant to that PR:
> Examples would be the [BlockManager::SaveBlockToDisk](https://github.com/ryanofsky/bitcoin/blob/4d2c9de24916f8d69514ea7c7251136e2762fa5c/src/node/blockstorage.cpp#L1167-L1195) function from https://github.com/bitcoin/bitcoin/pull/29700
Had just a quick look, but this looks like how I'd expect `Update()` to
...