💬 maflcko commented on pull request "doc: Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852604898)
lgtm ACK 1e0ed3bc0a99c08384642ad81ed4771bc98208b0
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852604898)
lgtm ACK 1e0ed3bc0a99c08384642ad81ed4771bc98208b0
💬 eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852632772)
> The fee rate was left out of the screenshots because it's not relevant to the analysis, but your example proves it's right all the same.
Okay, thanks for clarifying, now I understand the precise point being argued.
> That inscription is spending an $6.68 UTXO, and the transactor is spending a combined value of $2315. Both are equally competitive in the currently broken fee market (because they were included in the same block) despite the 2 orders of magnitude of difference between their val
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852632772)
> The fee rate was left out of the screenshots because it's not relevant to the analysis, but your example proves it's right all the same.
Okay, thanks for clarifying, now I understand the precise point being argued.
> That inscription is spending an $6.68 UTXO, and the transactor is spending a combined value of $2315. Both are equally competitive in the currently broken fee market (because they were included in the same block) despite the 2 orders of magnitude of difference between their val
...
💬 shovas commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1852635635)
Is there a workaround in the meantime?
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1852635635)
Is there a workaround in the meantime?
📝 maflcko opened a pull request: " test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067)
`struct` has many issues in messages.py:
* For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
* For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
* For packing and unpacking of a single value, the format string consists of charac
...
(https://github.com/bitcoin/bitcoin/pull/29067)
`struct` has many issues in messages.py:
* For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
* For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
* For packing and unpacking of a single value, the format string consists of charac
...
💬 furszy commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1852665616)
> Is there a workaround in the meantime?
Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue?
Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
Just as a small advice (not the best as your node should just work fine but.. having a backup plan always helps); you could have a full chain backup in a different storage. Then, if needed, transfer it to your pruned node to quickly recover from any storage corruption
...
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1852665616)
> Is there a workaround in the meantime?
Would be good to know why your node is aborting on a bad block. Maybe you have a storage issue?
Could you share the logs? (here or in a new issue is fine, just tag me when you do it).
Just as a small advice (not the best as your node should just work fine but.. having a backup plan always helps); you could have a full chain backup in a different storage. Then, if needed, transfer it to your pruned node to quickly recover from any storage corruption
...
💬 1ma commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852673575)
The precise point at which a fee "perverts" is a red herring, IMO.
If all TXs in the mempool are trying to move sats while economizing the fee as much as possible the free market just finds the % at which transactors are willing to transact, be it 0.8%, or 3% or whatever.
The problem arises when you allow *in the same market* a new kind of TX interested in another use case which doesn't mind straight up paying >95% of fees, because they're doing something else. They quickly displace the in
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852673575)
The precise point at which a fee "perverts" is a red herring, IMO.
If all TXs in the mempool are trying to move sats while economizing the fee as much as possible the free market just finds the % at which transactors are willing to transact, be it 0.8%, or 3% or whatever.
The problem arises when you allow *in the same market* a new kind of TX interested in another use case which doesn't mind straight up paying >95% of fees, because they're doing something else. They quickly displace the in
...
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1852679820)
Thanks for the review vasild!. Will tackle all points in the coming days. I'm currently finishing few bug fixes priorities and will be fully here again.
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1852679820)
Thanks for the review vasild!. Will tackle all points in the coming days. I'm currently finishing few bug fixes priorities and will be fully here again.
🤔 sdaftuar reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1777611474)
I read the code more carefully (other than the tests, which I just skimmed) -- so far this looks good, just had a few comments. Planning to do some more thorough testing next.
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1777611474)
I read the code more carefully (other than the tests, which I just skimmed) -- so far this looks good, just had a few comments. Planning to do some more thorough testing next.
💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424358331)
Perhaps a comment here would be helpful to explain why this change is needed.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424358331)
Perhaps a comment here would be helpful to explain why this change is needed.
💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424084885)
nit: I found the "not" in this comment confusing.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424084885)
nit: I found the "not" in this comment confusing.
💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424077858)
Is there a reason to drop the `txns.size() > 1` test in this `if` clause?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424077858)
Is there a reason to drop the `txns.size() > 1` test in this `if` clause?
💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424392025)
I was trying to figure out exactly why this code is safe in a package RBF context. I think for a generalized package RBF, this would not be safe, because we do not verify that a child transaction in an incoming package (say) isn't spending a coin in the mempool that would be conflicted by some other transaction in the package.
However, in this particular case, we are enforcing in `PackageMempoolChecks()` (further down) that the incoming package has **no** other in-mempool ancestors (ie the ne
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424392025)
I was trying to figure out exactly why this code is safe in a package RBF context. I think for a generalized package RBF, this would not be safe, because we do not verify that a child transaction in an incoming package (say) isn't spending a coin in the mempool that would be conflicted by some other transaction in the package.
However, in this particular case, we are enforcing in `PackageMempoolChecks()` (further down) that the incoming package has **no** other in-mempool ancestors (ie the ne
...
🤔 ryanofsky reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1778060101)
Updated 6d8cb4d4cd6062ad06cd351439e9139e028995d3 -> 9ba4dc41fd405a008e9badc4092ef1905f534b96 ([`pr/rmutil.6`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.6) -> [`pr/rmutil.7`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.6..pr/rmutil.7)) fixing crypto descriptions in libraries.md
re: https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307
>* moving the spanparsing stuff to script mo
...
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1778060101)
Updated 6d8cb4d4cd6062ad06cd351439e9139e028995d3 -> 9ba4dc41fd405a008e9badc4092ef1905f534b96 ([`pr/rmutil.6`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.6) -> [`pr/rmutil.7`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.6..pr/rmutil.7)) fixing crypto descriptions in libraries.md
re: https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307
>* moving the spanparsing stuff to script mo
...
💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337096)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395
> Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?
Yes that should be right. I think I just deluded myself because I didn't want to update the mermaid diagram. Updated now though.
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337096)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395
> Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?
Yes that should be right. I think I just deluded myself because I didn't want to update the mermaid diagram. Updated now though.
💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337693)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364
> It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?
>
> `TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.
>
> So putting this in common seems more sensible to me?
This is sort of a borderline case, and I did consider whether to move it in t
...
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337693)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364
> It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?
>
> `TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.
>
> So putting this in common seems more sensible to me?
This is sort of a borderline case, and I did consider whether to move it in t
...
💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852688371)
re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1851506064
> Maybe removing `sock.cpp` is a bad example, since it is already impossible to link against it. `sock.cpp` uses modules that are already not part of the kernel, so trying to link it in will result in errors. Then again, if `sock.cpp` uses common modules, why is it in `util` in the first place?
I'm actually not sure what is this referring to. Running `nm ./src/util/libbitcoin_util_a-sock.o --undefined-only` I see soc
...
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852688371)
re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1851506064
> Maybe removing `sock.cpp` is a bad example, since it is already impossible to link against it. `sock.cpp` uses modules that are already not part of the kernel, so trying to link it in will result in errors. Then again, if `sock.cpp` uses common modules, why is it in `util` in the first place?
I'm actually not sure what is this referring to. Running `nm ./src/util/libbitcoin_util_a-sock.o --undefined-only` I see soc
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424495073)
It's essentially an existing no-op. IIRC `AcceptMultipleTransactions` is only called in *unit tests* with size 1, otherwise it's always > 1.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424495073)
It's essentially an existing no-op. IIRC `AcceptMultipleTransactions` is only called in *unit tests* with size 1, otherwise it's always > 1.
💬 hebasto commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691455)
Oops... Forgot to update a version in the `AX_BOOST_BASE` macro.
Fixed now.
Sorry.
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691455)
Oops... Forgot to update a version in the `AX_BOOST_BASE` macro.
Fixed now.
Sorry.
💬 fanquake commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691556)
> This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.
I know a [WIP] branch for this "exists" in some form. cc @theuni
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691556)
> This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.
I know a [WIP] branch for this "exists" in some form. cc @theuni
🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1777495734)
Updated eaf915d61d470372e63f41f11d6a873c1936f73f -> 6db04be102807ee0120981a9b8de62a55439dabb ([`pr/noshut.22`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.22) -> [`pr/noshut.23`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.22..pr/noshut.23)) just making suggested code tweaks and updating all commit messages to say what behavior is changing.
re: https://github.com/bitcoin/bitcoin/pull/28051#pullrequestr
...
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1777495734)
Updated eaf915d61d470372e63f41f11d6a873c1936f73f -> 6db04be102807ee0120981a9b8de62a55439dabb ([`pr/noshut.22`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.22) -> [`pr/noshut.23`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.22..pr/noshut.23)) just making suggested code tweaks and updating all commit messages to say what behavior is changing.
re: https://github.com/bitcoin/bitcoin/pull/28051#pullrequestr
...