💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415131623)
They are unobservable side-effects.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415131623)
They are unobservable side-effects.
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415133326)
yes, with current versions it would use the max size, it's why I wrote "we would likely need some tricks to save memory". I don't have a concrete better idea yet but will try to come up with something better.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415133326)
yes, with current versions it would use the max size, it's why I wrote "we would likely need some tricks to save memory". I don't have a concrete better idea yet but will try to come up with something better.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415139924)
Yes, or when `BlockBuilder` objects exist. All functions in the `TxGraph` interface declare when they're allowed to be called.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415139924)
Yes, or when `BlockBuilder` objects exist. All functions in the `TxGraph` interface declare when they're allowed to be called.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415143034)
We're using the name `SetType` in many places in the cluster linearization code.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415143034)
We're using the name `SetType` in many places in the cluster linearization code.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415145512)
The code already uses `final` on all of the `TxGraphImpl` functions. Will leave for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415145512)
The code already uses `final` on all of the `TxGraphImpl` functions. Will leave for a follow-up.
💬 achow101 commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415146156)
It's actually not necessary to add it as a remote. You can give the repo url directly and it will fetch as needed. This avoids adding a bunch of unnecessary refs to your repo.
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415146156)
It's actually not necessary to add it as a remote. You can give the repo url directly and it will fetch as needed. This avoids adding a bunch of unnecessary refs to your repo.
💬 earonesty commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383559491)
>
> If JPEG-degens turn out to be the highest paying user segment, maybe
> humanity has failed Bitcoin?
>
Currently, fee-bearing transactions are not occurring at a high rate for
financial transactions because there is a lower-demand for large-value,
slow-settlement transactions than there is for instant transactions.
While lightning resolves much of this, so many implementations are
custodial (reasonable for a low values) and rarely clear to the chain.
Focusing on improving layer-2 accessibi
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383559491)
>
> If JPEG-degens turn out to be the highest paying user segment, maybe
> humanity has failed Bitcoin?
>
Currently, fee-bearing transactions are not occurring at a high rate for
financial transactions because there is a lower-demand for large-value,
slow-settlement transactions than there is for instant transactions.
While lightning resolves much of this, so many implementations are
custodial (reasonable for a low values) and rarely clear to the chain.
Focusing on improving layer-2 accessibi
...
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415235592)
The question was referring to the documentation of `NEEDS_SPLIT`
```
/** This cluster may have multiple disconnected components, which are all NEEDS_RELINEARIZE. */
NEEDS_SPLIT,
``
and was asking if this still holds in every case (even for small clusters)
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415235592)
The question was referring to the documentation of `NEEDS_SPLIT`
```
/** This cluster may have multiple disconnected components, which are all NEEDS_RELINEARIZE. */
NEEDS_SPLIT,
``
and was asking if this still holds in every case (even for small clusters)
💬 Dorex45 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383602356)
@earonesty
I don’t believe humanity has “failed” Bitcoin just because certain segments like JPEG-degens are experimenting and paying high fees. Bitcoin was never about dictating use cases; it was about open access, permissionless innovation, and monetary sovereignty. What we’re seeing is simply "market discovery on-chain.""
But here’s the real issue , Bitcoin’s base layer was never meant to host high-volume or high-frequency applications. It’s a settlement layer, and anything that tries to pus
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383602356)
@earonesty
I don’t believe humanity has “failed” Bitcoin just because certain segments like JPEG-degens are experimenting and paying high fees. Bitcoin was never about dictating use cases; it was about open access, permissionless innovation, and monetary sovereignty. What we’re seeing is simply "market discovery on-chain.""
But here’s the real issue , Bitcoin’s base layer was never meant to host high-volume or high-frequency applications. It’s a settlement layer, and anything that tries to pus
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415494808)
Maybe it could say "which will all be made NEEDS_RELINEARIZE". It's always possible (even for non-tiny clusters) that nothing really needs to be done with it anymore.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415494808)
Maybe it could say "which will all be made NEEDS_RELINEARIZE". It's always possible (even for non-tiny clusters) that nothing really needs to be done with it anymore.
🤔 kannapoix reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3317125427)
code review ACK: f45b94d302a
tests pass locally. I left a nit/question about mocktime.
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3317125427)
code review ACK: f45b94d302a
tests pass locally. I left a nit/question about mocktime.
💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2415489353)
nit: Do we still need this mocktime? The tests seem to pass without it.
If it is necessary, could we use a more understandable value? Previously it was chosen to minimize diffs in generated test data, but that constraint no longer applies here: https://github.com/bitcoin/bitcoin/pull/14802#discussion_r279476692
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2415489353)
nit: Do we still need this mocktime? The tests seem to pass without it.
If it is necessary, could we use a more understandable value? Previously it was chosen to minimize diffs in generated test data, but that constraint no longer applies here: https://github.com/bitcoin/bitcoin/pull/14802#discussion_r279476692
💬 maflcko commented on pull request "ci: Properly include $FILE_ENV in DEPENDS_HASH":
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384368809)
No idea what the unrelated intermittent CI failure is about: https://github.com/bitcoin/bitcoin/actions/runs/18358988770/job/52298824086?pr=33581#step:12:287 It just seems to stop during the very first init of bitcoind.exe.
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384368809)
No idea what the unrelated intermittent CI failure is about: https://github.com/bitcoin/bitcoin/actions/runs/18358988770/job/52298824086?pr=33581#step:12:287 It just seems to stop during the very first init of bitcoind.exe.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3384371268)
Updated 4fce466d1a3e345836434f8fb375980f626981f1 -> 2a80cd9f1db45dfc1775ded6698b034691e7bf2d ([kernelApi_71](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_71) -> [kernelApi_72](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_72), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_71..kernelApi_72))
* Fixed windows CI errors by excluding bitcoin-chainstate and test_kernel from manifest checks.
* Added `inline` to logging function definitions in the wrapper.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3384371268)
Updated 4fce466d1a3e345836434f8fb375980f626981f1 -> 2a80cd9f1db45dfc1775ded6698b034691e7bf2d ([kernelApi_71](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_71) -> [kernelApi_72](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_72), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_71..kernelApi_72))
* Fixed windows CI errors by excluding bitcoin-chainstate and test_kernel from manifest checks.
* Added `inline` to logging function definitions in the wrapper.
💬 maflcko commented on pull request "ci: Properly include $FILE_ENV in DEPENDS_HASH":
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384392670)
Nice catch. Tested via:
```
sh-5.2$ ( export FILE_ENV="./ci/test/00_setup_env_win64.sh" && git ls-tree HEAD depends "ci/test/$FILE_ENV" | sha256sum | cut -d' ' -f1 )
9e598ae5c5f89396b330088c224cd5e1b391f8d101906c0270a48fa3acbdce07
sh-5.2$ ( export FILE_ENV="./ci/test/00_setup_env_win64.sh" && git ls-tree HEAD depends "$FILE_ENV" | sha256sum | cut -d' ' -f1 )
6e4783c2f98f56e05e4b0d535b1709fce7c5f318a67932b0c8ed7a34b0a7ffbb
```
First the erroneous hash, and then the intended hash, bot
...
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384392670)
Nice catch. Tested via:
```
sh-5.2$ ( export FILE_ENV="./ci/test/00_setup_env_win64.sh" && git ls-tree HEAD depends "ci/test/$FILE_ENV" | sha256sum | cut -d' ' -f1 )
9e598ae5c5f89396b330088c224cd5e1b391f8d101906c0270a48fa3acbdce07
sh-5.2$ ( export FILE_ENV="./ci/test/00_setup_env_win64.sh" && git ls-tree HEAD depends "$FILE_ENV" | sha256sum | cut -d' ' -f1 )
6e4783c2f98f56e05e4b0d535b1709fce7c5f318a67932b0c8ed7a34b0a7ffbb
```
First the erroneous hash, and then the intended hash, bot
...
💬 Sjors commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415867744)
We also recommend this for the verification process and I find it much more convenient than looking up the full URL. I haven't run out of refs :-)
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415867744)
We also recommend this for the verification process and I find it much more convenient than looking up the full URL. I haven't run out of refs :-)
👍 hodlinator approved a pull request: "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions"
(https://github.com/bitcoin/bitcoin/pull/33569#pullrequestreview-3317673822)
crACK ec2476c8115e249ddfa3c8a1e8c892d5f822167b
(https://github.com/bitcoin/bitcoin/pull/33569#pullrequestreview-3317673822)
crACK ec2476c8115e249ddfa3c8a1e8c892d5f822167b
💬 hodlinator commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2415901630)
Thanks!
Nice mention regarding clang-tidy's misc-throw-by-value-catch-by-reference allowing for throwing & catching string literals.
### ++nit
Now that there is a proposed fix in https://github.com/llvm/llvm-project/pull/162546, we can see that although the parent issue to this one (#33550) is about libc++, that is not the case here. Going by the llvm-project PR, the issue is in the Clang compiler's code generation (when compiling for Windows which uses SEH).
Commit message could be made mor
...
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2415901630)
Thanks!
Nice mention regarding clang-tidy's misc-throw-by-value-catch-by-reference allowing for throwing & catching string literals.
### ++nit
Now that there is a proposed fix in https://github.com/llvm/llvm-project/pull/162546, we can see that although the parent issue to this one (#33550) is about libc++, that is not the case here. Going by the llvm-project PR, the issue is in the Clang compiler's code generation (when compiling for Windows which uses SEH).
Commit message could be made mor
...
💬 hodlinator commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2415923655)
meganit: Uses `()` in 3 `throw`s and `{}` in 3.
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2415923655)
meganit: Uses `()` in 3 `throw`s and `{}` in 3.
💬 rkrux commented on pull request "wallet: Store transactions in a separate sqlite table":
(https://github.com/bitcoin/bitcoin/pull/33034#issuecomment-3384679811)
Concept ACK a25557a
I have gone through the PR description and have skimmed over the commit messages as of now. At the outset, I find myself agreeing with the intent here. I am not aware of a strong enough reason to keep using SQLite as a KVS for all purposes, using its relational database properties for wallet transactions seem like a good start.
I did notice some serialisation specifics of transaction properties in a couple previous PRs that I didn't fully understand why were required; it
...
(https://github.com/bitcoin/bitcoin/pull/33034#issuecomment-3384679811)
Concept ACK a25557a
I have gone through the PR description and have skimmed over the commit messages as of now. At the outset, I find myself agreeing with the intent here. I am not aware of a strong enough reason to keep using SQLite as a KVS for all purposes, using its relational database properties for wallet transactions seem like a good start.
I did notice some serialisation specifics of transaction properties in a couple previous PRs that I didn't fully understand why were required; it
...