💬 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
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580997920)
nit: unnecessary extra newline
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580997920)
nit: unnecessary extra newline
👍 stickies-v approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2024988118)
ACK e1be443315be6ba6c80267e0e6be59deee77de50
> The use-case for update is when you have a function that is performing multiple operations and you want to accumulate warnings and errors from all the operations and return them.
I agree, and from what I've seen so far, I would prefer that to be the only use-case.
> I also looked at remaining two update calls in this PR, and thought they both were not good uses, so I removed them as well.
Ah, nice one. Given that chaining results is not
...
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2024988118)
ACK e1be443315be6ba6c80267e0e6be59deee77de50
> The use-case for update is when you have a function that is performing multiple operations and you want to accumulate warnings and errors from all the operations and return them.
I agree, and from what I've seen so far, I would prefer that to be the only use-case.
> I also looked at remaining two update calls in this PR, and thought they both were not good uses, so I removed them as well.
Ah, nice one. Given that chaining results is not
...
💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581037929)
> Not sure about changing the code for a broken compiler warning.
It's also slightly more readable because it checks `window_tx_count` only once.
> I am sure this warning already exists in other parts of the code
This is the only warning thrown.
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581037929)
> Not sure about changing the code for a broken compiler warning.
It's also slightly more readable because it checks `window_tx_count` only once.
> I am sure this warning already exists in other parts of the code
This is the only warning thrown.
💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581040393)
> This is the only warning thrown.
What about all the ones I get, or others? For example: https://api.cirrus-ci.com/v1/task/5983141741985792/logs/make.log
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581040393)
> This is the only warning thrown.
What about all the ones I get, or others? For example: https://api.cirrus-ci.com/v1/task/5983141741985792/logs/make.log
💬 kevkevinpal commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2079403879)
ACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)
looks good to me
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2079403879)
ACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)
looks good to me
⚠️ sdaftuar opened an issue: "qa: Support git worktrees when running the linters locally via Docker"
(https://github.com/bitcoin/bitcoin/issues/29972)
### Please describe the feature you'd like to see added.
I discovered that running this command (from `test/lint/README.md`):
```DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter```
doesn't work when trying to run it out of a git worktree:
```
fatal: not a git repository: /home/sdaftuar/bitcoin/.git/worktrees/2024-04-cluster-mempool-txgraph-rebased
```
(Though it did work for me when I tried checking
...
(https://github.com/bitcoin/bitcoin/issues/29972)
### Please describe the feature you'd like to see added.
I discovered that running this command (from `test/lint/README.md`):
```DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter```
doesn't work when trying to run it out of a git worktree:
```
fatal: not a git repository: /home/sdaftuar/bitcoin/.git/worktrees/2024-04-cluster-mempool-txgraph-rebased
```
(Though it did work for me when I tried checking
...
📝 alfonsoromanz opened a pull request: "[Test] Assumeutxo: ensure failure when importing a snapshot twice"
(https://github.com/bitcoin/bitcoin/pull/29973)
I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate.
(https://github.com/bitcoin/bitcoin/pull/29973)
I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate.