💬 0xBEEFCAF3 commented on pull request "Reenable OP_CAT":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-1922343699)
Drafting until I get the [inquisition PR](https://github.com/bitcoin-inquisition/bitcoin/pull/39) approved and I can get the builds passing.
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-1922343699)
Drafting until I get the [inquisition PR](https://github.com/bitcoin-inquisition/bitcoin/pull/39) approved and I can get the builds passing.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475234088)
I don't remember exactly what the bug was, but I know that it had something to do with assuming that all transactions that we call `transactionRemovedFromMempool` on are `TxStateInMempool`. That being said, I will change it back to the way it was before because it is not causing any problems right now.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475234088)
I don't remember exactly what the bug was, but I know that it had something to do with assuming that all transactions that we call `transactionRemovedFromMempool` on are `TxStateInMempool`. That being said, I will change it back to the way it was before because it is not causing any problems right now.
📝 theuni converted_to_draft a pull request: "serialization: c++20 endian/byteswap/clz modernization"
(https://github.com/bitcoin/bitcoin/pull/29263)
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking
...
(https://github.com/bitcoin/bitcoin/pull/29263)
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking
...
💬 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.