🤔 brunoerg reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2635611287)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2635611287)
Concept ACK
⚠️ TheBlueMatt opened an issue: "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols"
(https://github.com/bitcoin/bitcoin/issues/31938)
In lightning, we often want to deal with HTLCs which are below the dust threshold and thus must be burned to fees (absent something better to do with them). When doing ephemeral anchors, we want to retain that property. Ephemeral dust currently, however, requires that we burn them into the anchor (as we aren't allowed to have a non-0 fee when dealing with ephemeral dust).
If we burn the entire dust HTLC sum (which may be nontrivial, though it should never be huge) into the anchor (and fees are
...
(https://github.com/bitcoin/bitcoin/issues/31938)
In lightning, we often want to deal with HTLCs which are below the dust threshold and thus must be burned to fees (absent something better to do with them). When doing ephemeral anchors, we want to retain that property. Ephemeral dust currently, however, requires that we burn them into the anchor (as we aren't allowed to have a non-0 fee when dealing with ephemeral dust).
If we burn the entire dust HTLC sum (which may be nontrivial, though it should never be huge) into the anchor (and fees are
...
💬 mahmoudeA commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1966818326)
Btc
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1966818326)
Btc
🤔 mahmoudeA reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2635663474)
src/wallet/wallet.h
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2635663474)
src/wallet/wallet.h
💬 mahmoudeA commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2676970105)
src/wallet/wallet.h
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2676970105)
src/wallet/wallet.h
📝 willcl-ark opened a pull request: "Add assumeutxo chainparams to release-process.md"
(https://github.com/bitcoin/bitcoin/pull/31940)
This should ideally be bumped every major (and perhaps even minor?) release to avoid falling too far behind, and therefore keeping this feature as useful as it can be.
Document in release-process.md to avoid forgetting to do this.
(https://github.com/bitcoin/bitcoin/pull/31940)
This should ideally be bumped every major (and perhaps even minor?) release to avoid falling too far behind, and therefore keeping this feature as useful as it can be.
Document in release-process.md to avoid forgetting to do this.
⚠️ RealStanima opened an issue: "Proposal for Bitcoin Expiration Mechanism to Improve Scalability and Maintain Mining Incentives"
(https://github.com/bitcoin/bitcoin/issues/31941)
### Please describe the feature you'd like to see added.
**Title**: Proposal for Bitcoin Expiration Mechanism to Improve Scalability and Maintain Mining Incentives
**Summary:**
This proposal suggests implementing an expiration mechanism for unmoved Bitcoin to address long-term scalability, lost coins, and mining incentives. Under this model, Bitcoin that has remained unspent for a fixed period (e.g., 16 years) would be reabsorbed into the mining pool as block rewards, ensuring the blockchain r
...
(https://github.com/bitcoin/bitcoin/issues/31941)
### Please describe the feature you'd like to see added.
**Title**: Proposal for Bitcoin Expiration Mechanism to Improve Scalability and Maintain Mining Incentives
**Summary:**
This proposal suggests implementing an expiration mechanism for unmoved Bitcoin to address long-term scalability, lost coins, and mining incentives. Under this model, Bitcoin that has remained unspent for a fixed period (e.g., 16 years) would be reabsorbed into the mining pool as block rewards, ensuring the blockchain r
...
✅ fanquake closed an issue: "Proposal for Bitcoin Expiration Mechanism to Improve Scalability and Maintain Mining Incentives"
(https://github.com/bitcoin/bitcoin/issues/31941)
(https://github.com/bitcoin/bitcoin/issues/31941)
⚠️ maflcko opened an issue: "[rfc] build: Reject unclean configure?"
(https://github.com/bitcoin/bitcoin/issues/31942)
When re-configuring a build, it is possible to do so in an unclean way on a stale build folder.
The average person initiating such a build does not know when a stale build is safe and when not. Time is wasted for the builder when a broken build silently succeeds and then leads to a nonsensical error later on. Just to give the last two examples of the last few days: (https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781, https://github.com/bitcoin/bitcoin/pull/31933#discussion_
...
(https://github.com/bitcoin/bitcoin/issues/31942)
When re-configuring a build, it is possible to do so in an unclean way on a stale build folder.
The average person initiating such a build does not know when a stale build is safe and when not. Time is wasted for the builder when a broken build silently succeeds and then leads to a nonsensical error later on. Just to give the last two examples of the last few days: (https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781, https://github.com/bitcoin/bitcoin/pull/31933#discussion_
...
💬 maflcko commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967220774)
> For MacOs (15.3.1 M4 pro) , I have llvm 19 which is also a known [issue](https://github.com/bitcoin/bitcoin/issues/31049) so I generally have to specify my commands with this to use llvm 18.
Is this issue specific to building the fuzz tests, or just a general issue? From your command, it seems like it is a general issue. In this case, I wonder if it makes sense to mention here that all binaries (`clang`, `clang++`, `llvm-cov`, `llvm-profdata`) will have to be called via `$(brew --prefix llv
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967220774)
> For MacOs (15.3.1 M4 pro) , I have llvm 19 which is also a known [issue](https://github.com/bitcoin/bitcoin/issues/31049) so I generally have to specify my commands with this to use llvm 18.
Is this issue specific to building the fuzz tests, or just a general issue? From your command, it seems like it is a general issue. In this case, I wonder if it makes sense to mention here that all binaries (`clang`, `clang++`, `llvm-cov`, `llvm-profdata`) will have to be called via `$(brew --prefix llv
...
💬 maflcko commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2677764949)
Would it make sense to temporarily pin the clang version to 18, in https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer, for now? Otherwise, people will keep running into this?
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2677764949)
Would it make sense to temporarily pin the clang version to 18, in https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer, for now? Otherwise, people will keep running into this?
🤔 maflcko reviewed a pull request: "log,optimization: use original log string when no suspicious chars found"
(https://github.com/bitcoin/bitcoin/pull/31923#pullrequestreview-2636342094)
Looks like part of the changes are based on a misunderstanding, so I left a comment.
However, I am not sure if the changes should be done. If there is no end-user or developer-facing goal or benefits, then not changing the code seems preferable.
(https://github.com/bitcoin/bitcoin/pull/31923#pullrequestreview-2636342094)
Looks like part of the changes are based on a misunderstanding, so I left a comment.
However, I am not sure if the changes should be done. If there is no end-user or developer-facing goal or benefits, then not changing the code seems preferable.
💬 maflcko commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#discussion_r1967231445)
This doesn't use the original string. It still creates a copy and changing `std::string_view str` to `const std::string& str` in the function signature doesn't change that. In fact, (albeit not in this patch), the signature change is a pessimization, because the compiler may be forced to create a temporary string to be able to create a reference to it, when a plain string_view would be sufficient.
(https://github.com/bitcoin/bitcoin/pull/31923#discussion_r1967231445)
This doesn't use the original string. It still creates a copy and changing `std::string_view str` to `const std::string& str` in the function signature doesn't change that. In fact, (albeit not in this patch), the signature change is a pessimization, because the compiler may be forced to create a temporary string to be able to create a reference to it, when a plain string_view would be sufficient.
💬 maflcko commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2677806174)
Trying to access https://corecheck.dev/bitcoin/bitcoin/pulls/31923 gives me
```
This function has crashed
An unhandled error in the function code triggered the following message:
LambdaTimeout
```
cc @m3dwards
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2677806174)
Trying to access https://corecheck.dev/bitcoin/bitcoin/pulls/31923 gives me
```
This function has crashed
An unhandled error in the function code triggered the following message:
LambdaTimeout
```
cc @m3dwards
💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2677821439)
Hey,
Sorry for the delayed updates to this PR, I just force pushed after quash the split Base58 and bech32 decoding portions. Per the request of @Sjors .
There also was a substantial effort to create a proper set of functional tests that actually test properly. Examples would be that now with the correct network decoding some errors became network aware and as such needed to be converted to the correct network. Example would be the bech32 padding issue. test/functional/rpc_validateaddres
...
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2677821439)
Hey,
Sorry for the delayed updates to this PR, I just force pushed after quash the split Base58 and bech32 decoding portions. Per the request of @Sjors .
There also was a substantial effort to create a proper set of functional tests that actually test properly. Examples would be that now with the correct network decoding some errors became network aware and as such needed to be converted to the correct network. Example would be the bech32 padding issue. test/functional/rpc_validateaddres
...
💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967280678)
I prefer:
- Keeping my shell in the root of the repo
- Not accumulating lingering environment variables
How about:
```suggestion
LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```
(`ctest --test-dir build` is also how we recommend running tests in a bunch of places).
It would also require this below:
`find build -name "default_*.profraw"`
`llvm-cov show build/src/test/test_bitcoin`
Might also want to change `--output-dir` for
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967280678)
I prefer:
- Keeping my shell in the root of the repo
- Not accumulating lingering environment variables
How about:
```suggestion
LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```
(`ctest --test-dir build` is also how we recommend running tests in a bunch of places).
It would also require this below:
`find build -name "default_*.profraw"`
`llvm-cov show build/src/test/test_bitcoin`
Might also want to change `--output-dir` for
...
🤔 hodlinator reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2636430240)
Concept ACK 6436bdb301278abb3526eab2138b2f23f8905b5e
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2636430240)
Concept ACK 6436bdb301278abb3526eab2138b2f23f8905b5e
💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967320704)
What do you mean by "Default" here? Are your distinguishing from "XCode-special-sauce-LLVM/Clang"? I would have referred to it as "mainline" or "vanilla" in that case.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1967320704)
What do you mean by "Default" here? Are your distinguishing from "XCode-special-sauce-LLVM/Clang"? I would have referred to it as "mainline" or "vanilla" in that case.
📝 rkrux opened a pull request: "test: add coverage for abandoning unconfirmed transaction"
(https://github.com/bitcoin/bitcoin/pull/31943)
Previous discussion: https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
Current Coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html#L1326
(https://github.com/bitcoin/bitcoin/pull/31943)
Previous discussion: https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
Current Coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html#L1326
💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967400390)
A minor improvement could be to at least add ` \`.
```suggestion
To execute the tool, compilation has to be done with the build options
`-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' \
-DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'`. Both
llvm-profdata and llvm-cov must be installed.
```
But the beginning/end of lines would then still contain text one doesn't want to copy, so I prefer the first suggestion.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967400390)
A minor improvement could be to at least add ` \`.
```suggestion
To execute the tool, compilation has to be done with the build options
`-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' \
-DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'`. Both
llvm-profdata and llvm-cov must be installed.
```
But the beginning/end of lines would then still contain text one doesn't want to copy, so I prefer the first suggestion.