💬 Sjors commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569056045)
> Could you clarify the effect of setting it to 4000 in light of the recent findings?
It's still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn't detect in the analysis.
In the example of Ocean it _seems_ that their custom `-blockmaxweight` is low en
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569056045)
> Could you clarify the effect of setting it to 4000 in light of the recent findings?
It's still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn't detect in the analysis.
In the example of Ocean it _seems_ that their custom `-blockmaxweight` is low en
...
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901686821)
> I don't think it would be possible to write a clang-tidy plugin to check for these cases
I haven't tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any `FunctionTemplateDecl` whose `FunctionDecl` does not have any `TemplateTypeParm` in the AST should be sufficient to find those that can be written without `template<...>`.
Example: https://godbolt.org/z/4rre4rPT6
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901686821)
> I don't think it would be possible to write a clang-tidy plugin to check for these cases
I haven't tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any `FunctionTemplateDecl` whose `FunctionDecl` does not have any `TemplateTypeParm` in the AST should be sufficient to find those that can be written without `template<...>`.
Example: https://godbolt.org/z/4rre4rPT6
💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1901720687)
> The chainman reset may trigger validation interface notifications
Ah, right, `ChainStateFlushed` would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of `SyncWithValidationInterfaceQueue()` being called, so I suppose this doesn't really change anything wrt callbacks assuming `m_node.chainman` exists.
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1901720687)
> The chainman reset may trigger validation interface notifications
Ah, right, `ChainStateFlushed` would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of `SyncWithValidationInterfaceQueue()` being called, so I suppose this doesn't really change anything wrt callbacks assuming `m_node.chainman` exists.
🚀 glozow merged a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121)
(https://github.com/bitcoin/bitcoin/pull/28121)
🤔 maflcko requested changes to a pull request: "ci: Switch to latest macOS and Windows images"
(https://github.com/bitcoin/bitcoin/pull/31597#pullrequestreview-2528979701)
not sure about this. Is there a reason to do this?
(https://github.com/bitcoin/bitcoin/pull/31597#pullrequestreview-2528979701)
not sure about this. Is there a reason to do this?
💬 maflcko commented on pull request "ci: Switch to latest macOS and Windows images":
(https://github.com/bitcoin/bitcoin/pull/31597#discussion_r1901724818)
Not sure about this. This should be the minimum version supported. IIRC xcode 15 corresponds to clang-16, somewhat?
If you want to test the latest xcode version, you can add a different task here, or in a nightly repo.
Otherwise, the build may break silently for xcode 15 and users/devs will complain.
(https://github.com/bitcoin/bitcoin/pull/31597#discussion_r1901724818)
Not sure about this. This should be the minimum version supported. IIRC xcode 15 corresponds to clang-16, somewhat?
If you want to test the latest xcode version, you can add a different task here, or in a nightly repo.
Otherwise, the build may break silently for xcode 15 and users/devs will complain.
🤔 l0rinc reviewed a pull request: "qa: Ensure consistent use of decimals instead of floats"
(https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2528976834)
I'm not sure about this, aren't we just masking a bigger problem (i.e. treating floats as if they were real numbers).
Also, can we update any one of the CI tasks to use the latest python to make sure we don't have to fix these manually all the time?
(https://github.com/bitcoin/bitcoin/pull/31595#pullrequestreview-2528976834)
I'm not sure about this, aren't we just masking a bigger problem (i.e. treating floats as if they were real numbers).
Also, can we update any one of the CI tasks to use the latest python to make sure we don't have to fix these manually all the time?
💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901727037)
is this really better than before?
```suggestion
wallet.sendtoaddress(address=address, amount=1)
```
or
```suggestion
wallet.sendtoaddress(address=address, amount="1")
```
Both seem to work
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901727037)
is this really better than before?
```suggestion
wallet.sendtoaddress(address=address, amount=1)
```
or
```suggestion
wallet.sendtoaddress(address=address, amount="1")
```
Both seem to work
💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901722976)
Is it important this is a Decimal?
```suggestion
raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: "24.99"})
```
Seems to pass as well - floating point values weren't defined to be this precise anyway
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901722976)
Is it important this is a Decimal?
```suggestion
raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: "24.99"})
```
Seems to pass as well - floating point values weren't defined to be this precise anyway
💬 l0rinc commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901725082)
we have [many](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_signrawtransactionwithkey.py#L81) [other](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L291) [instances](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_bumpfee.py#L146) where we've kept the floating point format - can we set up a linter if that's important instead of fixing only a subset?
(https://github.com/bitcoin/bitcoin/pull/31595#discussion_r1901725082)
we have [many](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_signrawtransactionwithkey.py#L81) [other](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L291) [instances](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_bumpfee.py#L146) where we've kept the floating point format - can we set up a linter if that's important instead of fixing only a subset?
💬 maflcko commented on pull request "ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions":
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2569183122)
For reference, this conflicts with commit a1add80c80bc95afdd55652bb87379fb34150a5c from
> [#29881](https://github.com/bitcoin/bitcoin/pull/29881) (guix: use GCC 13 to build releases by fanquake)
However, this is fine, because any version 12, or above should be fine. I've edited the pull title to clarify GCC-12+.
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2569183122)
For reference, this conflicts with commit a1add80c80bc95afdd55652bb87379fb34150a5c from
> [#29881](https://github.com/bitcoin/bitcoin/pull/29881) (guix: use GCC 13 to build releases by fanquake)
However, this is fine, because any version 12, or above should be fine. I've edited the pull title to clarify GCC-12+.
💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901746082)
https://en.cppreference.com/w/cpp/container/vector/emplace_back#:~:text=It%20demonstrates%20how%20emplace_back%20forwards%20parameters%20to%20the%20President%20constructor
This is handy!
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901746082)
https://en.cppreference.com/w/cpp/container/vector/emplace_back#:~:text=It%20demonstrates%20how%20emplace_back%20forwards%20parameters%20to%20the%20President%20constructor
This is handy!
🤔 rkrux reviewed a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2529015468)
Nit: In the PR description, a draft PR is mentioned as the project. Can add this link for project issue: https://github.com/bitcoin/bitcoin/issues/30392
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2529015468)
Nit: In the PR description, a draft PR is mentioned as the project. Can add this link for project issue: https://github.com/bitcoin/bitcoin/issues/30392
💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901757147)
Getting thrown off a bit by the name here. Is it really a histogram at this point or just a vector of fee rates/fracs?
Going by this def: https://en.wikipedia.org/wiki/Histogram
Sorry for not raising this earlier.
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901757147)
Getting thrown off a bit by the name here. Is it really a histogram at this point or just a vector of fee rates/fracs?
Going by this def: https://en.wikipedia.org/wiki/Histogram
Sorry for not raising this earlier.
💬 rkrux commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901761122)
Might be helpful to add a comment here telling that the values in this vector are sorted. That's what I assumed while going through the `CalculatePercentiles` function here: https://github.com/bitcoin/bitcoin/pull/30157/files#diff-34fc4771c305399b6f7634acd8c5fd2d0b89115f998397d41676180d742c28daR10
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1901761122)
Might be helpful to add a comment here telling that the values in this vector are sorted. That's what I assumed while going through the `CalculatePercentiles` function here: https://github.com/bitcoin/bitcoin/pull/30157/files#diff-34fc4771c305399b6f7634acd8c5fd2d0b89115f998397d41676180d742c28daR10
📝 i-am-yuvi opened a pull request: "#31403 follow-up"
(https://github.com/bitcoin/bitcoin/pull/31598)
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
(https://github.com/bitcoin/bitcoin/pull/31598)
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
✅ i-am-yuvi closed a pull request: "#31403 follow-up"
(https://github.com/bitcoin/bitcoin/pull/31598)
(https://github.com/bitcoin/bitcoin/pull/31598)
📝 i-am-yuvi opened a pull request: "test: improve rogue calls in mining functions"
(https://github.com/bitcoin/bitcoin/pull/31599)
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
(https://github.com/bitcoin/bitcoin/pull/31599)
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
💬 richieoscar commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2569259288)
@adyshimony, this solution also worked for me
Installed LLVM via homebrew brew install llvm
Clean build / delete dir to clean cache via git clean -fxd
Updated the build command to use the Homebrew-installed LLVM path, and also adding the LLVM library path:
cmake -DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DCMAKE_EXE_LINKER_FLAGS=-L/opt/homebrew/opt/ll
...
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2569259288)
@adyshimony, this solution also worked for me
Installed LLVM via homebrew brew install llvm
Clean build / delete dir to clean cache via git clean -fxd
Updated the build command to use the Homebrew-installed LLVM path, and also adding the LLVM library path:
cmake -DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DCMAKE_EXE_LINKER_FLAGS=-L/opt/homebrew/opt/ll
...