💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777682322)
Feels like `TestNode` itself shouldn't have tx-level code. Maybe `/test/functional/test_framework/blocktools.py` would be slightly better? It has `create_witness_tx(node, ...)` and `send_to_witness(use_p2wsh, node, utxo, ...)`.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777682322)
Feels like `TestNode` itself shouldn't have tx-level code. Maybe `/test/functional/test_framework/blocktools.py` would be slightly better? It has `create_witness_tx(node, ...)` and `send_to_witness(use_p2wsh, node, utxo, ...)`.
💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777688749)
Could be applied to `getblock` in *src/rpc/blockchain.cpp* as well.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777688749)
Could be applied to `getblock` in *src/rpc/blockchain.cpp* as well.
💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777697752)
nit: The other 15 comments in the file do not occupy the same ratio of vertical space.
```suggestion
/** Allows providing orphan information externally */
```
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777697752)
nit: The other 15 comments in the file do not occupy the same ratio of vertical space.
```suggestion
/** Allows providing orphan information externally */
```
💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777729324)
nit: Might be nice to check the mempool size is zero as a post-condition, even though it should be covered by other tests.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777729324)
nit: Might be nice to check the mempool size is zero as a post-condition, even though it should be covered by other tests.
🤔 jarolrod reviewed a pull request: "contrib: Update asmap link in seeds readme"
(https://github.com/bitcoin/bitcoin/pull/30979#pullrequestreview-2332366220)
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
(https://github.com/bitcoin/bitcoin/pull/30979#pullrequestreview-2332366220)
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
📝 brunoerg opened a pull request: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984)
This PR adds a new field in `getpeerinfo` RPC to know whether a peer is registered for transaction reconciliation (we register it when receiving a `SENDTXRCNCL` msg).
- The field `tx_reconciliation` is optional for in case you do not support tx reconciliation.
- With this field, we can improve `p2p_sendtxrcncl` tests by checking whether a peer is registered for transaction reconciliation instead of simply rely on logs message.
- d660df07d756ba5e06b74e4b51a7287620ae306b - Just remove unnec
...
(https://github.com/bitcoin/bitcoin/pull/30984)
This PR adds a new field in `getpeerinfo` RPC to know whether a peer is registered for transaction reconciliation (we register it when receiving a `SENDTXRCNCL` msg).
- The field `tx_reconciliation` is optional for in case you do not support tx reconciliation.
- With this field, we can improve `p2p_sendtxrcncl` tests by checking whether a peer is registered for transaction reconciliation instead of simply rely on logs message.
- d660df07d756ba5e06b74e4b51a7287620ae306b - Just remove unnec
...
💬 brunoerg commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2377976569)
cc: @sr-gi per our convo.
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2377976569)
cc: @sr-gi per our convo.
👍 brunoerg approved a pull request: "contrib: Update asmap link in seeds readme"
(https://github.com/bitcoin/bitcoin/pull/30979#pullrequestreview-2332382359)
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
(https://github.com/bitcoin/bitcoin/pull/30979#pullrequestreview-2332382359)
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
⚠️ andremralves opened an issue: "gen-manpages.py should skip nonexistent binaries and still work for the existent binaries"
(https://github.com/bitcoin/bitcoin/issues/30985)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
By executing `BUILDDIR=$PWD/build contrib/devtools/gen-manpages.py`, if one binary is not found, it will stop the generation of all manpages.
For example: Executing gen-manpages.py without bitcoin-qt. The command exits after detecting that bitcoin-qt does not exist, and no manpage is generated, even though only bitcoin-qt is missing.
```bash
➜ bitcoin git:(master) ✗ BUILDDIR=$PWD/b
...
(https://github.com/bitcoin/bitcoin/issues/30985)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
By executing `BUILDDIR=$PWD/build contrib/devtools/gen-manpages.py`, if one binary is not found, it will stop the generation of all manpages.
For example: Executing gen-manpages.py without bitcoin-qt. The command exits after detecting that bitcoin-qt does not exist, and no manpage is generated, even though only bitcoin-qt is missing.
```bash
➜ bitcoin git:(master) ✗ BUILDDIR=$PWD/b
...
💬 glozow commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377982154)
Tend to agree with the "let's wait for cluster mempool, which is currently our only proper solution to the problem" approach.
My reasoning for #23121 was also that rule 2 doesn't actually stop people from adding "new" unconfirmed inputs, and (2) it doesn't do its job within the RBF rules of helping us check that replacements have a better mining score / would confirm faster. I think reason (2) is actually much more important and complicated. I'm not convinced that the removal would be an impr
...
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377982154)
Tend to agree with the "let's wait for cluster mempool, which is currently our only proper solution to the problem" approach.
My reasoning for #23121 was also that rule 2 doesn't actually stop people from adding "new" unconfirmed inputs, and (2) it doesn't do its job within the RBF rules of helping us check that replacements have a better mining score / would confirm faster. I think reason (2) is actually much more important and complicated. I'm not convinced that the removal would be an impr
...
📝 andremralves opened a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986)
Instead of stopping the execution of gen-manpages.py when a binary is not found, continue generating manpages for the available binaries and skip the missing ones.
closes #30985
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear moti
...
(https://github.com/bitcoin/bitcoin/pull/30986)
Instead of stopping the execution of gen-manpages.py when a binary is not found, continue generating manpages for the available binaries and skip the missing ones.
closes #30985
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear moti
...
📝 brunoerg converted_to_draft a pull request: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984)
This PR adds a new field in `getpeerinfo` RPC to know whether a peer is registered for transaction reconciliation (we register it when receiving a `SENDTXRCNCL` msg).
- The field `tx_reconciliation` is optional for in case you do not support tx reconciliation.
- With this field, we can improve `p2p_sendtxrcncl` tests by checking whether a peer is registered for transaction reconciliation instead of simply rely on logs message.
- d660df07d756ba5e06b74e4b51a7287620ae306b - Just remove unnec
...
(https://github.com/bitcoin/bitcoin/pull/30984)
This PR adds a new field in `getpeerinfo` RPC to know whether a peer is registered for transaction reconciliation (we register it when receiving a `SENDTXRCNCL` msg).
- The field `tx_reconciliation` is optional for in case you do not support tx reconciliation.
- With this field, we can improve `p2p_sendtxrcncl` tests by checking whether a peer is registered for transaction reconciliation instead of simply rely on logs message.
- d660df07d756ba5e06b74e4b51a7287620ae306b - Just remove unnec
...
📝 davidgumberg opened a pull request: "Don't zero-after-free `DataStream`: ~25% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/30987)
This PR modifies `DataStream`'s vector ` to use the default allocator `std::allocator` rather than `zero_after_free_allocator` which causes a major degradation to performance. The `zero_after_free_allocator` is identical to the default `std::allocator` except that it zeroes memory using `memory_cleanse()` before deallocating.
This PR also drops the `zero_after_free_allocator`, since this was only used by `DataStream` and `SerializeData`,
In my testing (n=2) on a Raspberry Pi 5 with 4GB of
...
(https://github.com/bitcoin/bitcoin/pull/30987)
This PR modifies `DataStream`'s vector ` to use the default allocator `std::allocator` rather than `zero_after_free_allocator` which causes a major degradation to performance. The `zero_after_free_allocator` is identical to the default `std::allocator` except that it zeroes memory using `memory_cleanse()` before deallocating.
This PR also drops the `zero_after_free_allocator`, since this was only used by `DataStream` and `SerializeData`,
In my testing (n=2) on a Raspberry Pi 5 with 4GB of
...
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777793921)
done
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777793921)
done
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777800911)
I think this was part of the first changes I did, I do like more the overloaded function approach as is less verbose without loosing readability. But later on I saw that we are using the `WithDB` suffix and changed the rest of the commits.
Will change `DeleteRecords` to use it. Thanks.
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777800911)
I think this was part of the first changes I did, I do like more the overloaded function approach as is less verbose without loosing readability. But later on I saw that we are using the `WithDB` suffix and changed the rest of the commits.
Will change `DeleteRecords` to use it. Thanks.
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777805371)
sure. done
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777805371)
sure. done
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777793995)
done
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777793995)
done
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777810516)
> in [c436b51](https://github.com/bitcoin/bitcoin/commit/c436b516450b35bb01393559d935e1b807e5a835): shouldn't those calls also happen as a batch transaction, i.e. wrapped around a TxnBegin/TxnCommit pair (at least in the context of this commit)?
yeah, they should, but that would make this commit and the last one more verbose without much gain. Maybe it's better to leave it as is for simplicity?
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1777810516)
> in [c436b51](https://github.com/bitcoin/bitcoin/commit/c436b516450b35bb01393559d935e1b807e5a835): shouldn't those calls also happen as a batch transaction, i.e. wrapped around a TxnBegin/TxnCommit pair (at least in the context of this commit)?
yeah, they should, but that would make this commit and the last one more verbose without much gain. Maybe it's better to leave it as is for simplicity?
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2378086007)
Updated per feedback, thanks @theStack!
> I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.
>
> Might be worth to update the PR descrip
...
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2378086007)
Updated per feedback, thanks @theStack!
> I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.
>
> Might be worth to update the PR descrip
...
🤔 pablomartin4btc reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2332500049)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2332500049)
Concept ACK