💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922418119)
Converted to a draft while I'm still messing with this.
I added a new commit to add the `bitcoin-config.h` include where it's necessary. I'm not done investigating that yet, but it's at least needed for `chacha20poly1305.cpp` as well.
The benchmarks still show some regressions, though it's not nearly as bad as it was before. Will continue poking.
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922418119)
Converted to a draft while I'm still messing with this.
I added a new commit to add the `bitcoin-config.h` include where it's necessary. I'm not done investigating that yet, but it's at least needed for `chacha20poly1305.cpp` as well.
The benchmarks still show some regressions, though it's not nearly as bad as it was before. Will continue poking.
📝 achow101 opened a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367)
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.
(https://github.com/bitcoin/bitcoin/pull/29367)
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.
💬 epiccurious commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922494863)
+1 to @1440000bytes, they can contact the listed people directly instead of `security@`.
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922494863)
+1 to @1440000bytes, they can contact the listed people directly instead of `security@`.
🤔 mzumsande reviewed a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1857950026)
ACK c340503b67585b631909584f1acaa327f5af25d3
I ran the functional test suite both with and without `--v2transport` on this branch and both runs succeeded.
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1857950026)
ACK c340503b67585b631909584f1acaa327f5af25d3
I ran the functional test suite both with and without `--v2transport` on this branch and both runs succeeded.
💬 epiccurious commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922597240)
utACK c340503b67585b631909584f1acaa327f5af25d3
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922597240)
utACK c340503b67585b631909584f1acaa327f5af25d3
💬 epiccurious commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1922599794)
Tested ACK. Passed the functional test runner.
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1922599794)
Tested ACK. Passed the functional test runner.
💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1922687448)
I'm working on it. Plan is to harden the function to not read at end and after. I have some initial small draft changes, and want to add testing. Maybe, we can even highlight the issue via additional fuzz+msan tests.
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1922687448)
I'm working on it. Plan is to harden the function to not read at end and after. I have some initial small draft changes, and want to add testing. Maybe, we can even highlight the issue via additional fuzz+msan tests.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475452360)
This could be done in a follow-up if people want it, because currently the conflicting transactions are stored in the `CWalletTx`, and not the transaction state, so this function is not aware of the txids of the mempool conflicts.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475452360)
This could be done in a follow-up if people want it, because currently the conflicting transactions are stored in the `CWalletTx`, and not the transaction state, so this function is not aware of the txids of the mempool conflicts.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453805)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453805)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453968)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453968)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454363)
Thanks, that should be fixed now
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454363)
Thanks, that should be fixed now
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454524)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454524)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454837)
Good point, the tests now check the exact contents.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454837)
Good point, the tests now check the exact contents.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475455088)
I've added an assertion that the tx isn't in the mempool.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475455088)
I've added an assertion that the tx isn't in the mempool.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475457535)
Yes, I will add these in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475457535)
Yes, I will add these in a follow-up PR.
💬 DanaBCannon commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1475536100)
Im Open For Business
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1475536100)
Im Open For Business
💬 stratospher commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922955178)
tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour!
there is also`p2p_leak.py`, `p2p_timeouts.py`, `p2p_tx_privacy.py` where `on_version()` is used with `add_p2p_connection()`/inbound to `TestNode` connections which aren't affected by this. I guess we should think about it only if we end up doing outbound to `TestNode` connections there.
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922955178)
tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour!
there is also`p2p_leak.py`, `p2p_timeouts.py`, `p2p_tx_privacy.py` where `on_version()` is used with `add_p2p_connection()`/inbound to `TestNode` connections which aren't affected by this. I guess we should think about it only if we end up doing outbound to `TestNode` connections there.
💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1923061666)
> @petertodd
>
> > Can you give a bit more detail on what challenges you think that'll pose?
>
> from my memory: "How this new replacement rule would behave if you have a parent in the "replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic. i wond
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1923061666)
> @petertodd
>
> > Can you give a bit more detail on what challenges you think that'll pose?
>
> from my memory: "How this new replacement rule would behave if you have a parent in the "replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic. i wond
...
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923151784)
> I've heard similar arguments from many people, and I have to say I disagree. The miners need to follow the will of the users (especially the holders), or bitcoin cannot survive long-term. Mining pools are highly attuned to short-term interests, generally at the expense of long-term considerations. This is understandable given the competitive and unpredictable nature of proof-of-work pooled mining. A pool operator who tries to be a good citizen and caretake bitcoin's long-term interests will mo
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923151784)
> I've heard similar arguments from many people, and I have to say I disagree. The miners need to follow the will of the users (especially the holders), or bitcoin cannot survive long-term. Mining pools are highly attuned to short-term interests, generally at the expense of long-term considerations. This is understandable given the competitive and unpredictable nature of proof-of-work pooled mining. A pool operator who tries to be a good citizen and caretake bitcoin's long-term interests will mo
...
💬 BitcoinMechanic commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923175031)
The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
As you point out, it is **extremely easy** to get three or four entities to do something like mempoolfullrbf=1.
The difference here is we aren't trying to route around network defaults, we are trying to have them be set
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923175031)
The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
As you point out, it is **extremely easy** to get three or four entities to do something like mempoolfullrbf=1.
The difference here is we aren't trying to route around network defaults, we are trying to have them be set
...