📝 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
💬 pablomartin4btc commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1777836170)
We have already [updated](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762156524) `<filename>` to `<walletname>` in a few places (`bitcoin-cli.cpp` and `wallet/rpc/utils.cpp`), shouldn't be this the case as well?
nit:
```suggestion
{"walletname", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the wallets directory), or a legacy wallet's .dat file (within the wallets directory). The default wallets directory
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1777836170)
We have already [updated](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762156524) `<filename>` to `<walletname>` in a few places (`bitcoin-cli.cpp` and `wallet/rpc/utils.cpp`), shouldn't be this the case as well?
nit:
```suggestion
{"walletname", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the wallets directory), or a legacy wallet's .dat file (within the wallets directory). The default wallets directory
...
💬 arik-so commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2378165487)
Thanks so much for the responses so far!
@sdaftuar, I'm not sure I fully follow your point regarding rule 6. Currently, can't the dummy transaction one creates to circumvent rule 2 simply have a really low fee rate? That would basically also create a situation akin to rule 2 not existing?
But one thing I'd be curious to hear some thoughts on is specifically the potential mempool bifurcation that circumventing rule 2 would create (taking into account that there obviously isn't such a thing
...
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2378165487)
Thanks so much for the responses so far!
@sdaftuar, I'm not sure I fully follow your point regarding rule 6. Currently, can't the dummy transaction one creates to circumvent rule 2 simply have a really low fee rate? That would basically also create a situation akin to rule 2 not existing?
But one thing I'd be curious to hear some thoughts on is specifically the potential mempool bifurcation that circumventing rule 2 would create (taking into account that there obviously isn't such a thing
...
💬 jarolrod commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2378206734)
> That makes sense to me, but i'm not user of the `.app`. I'm unfamiliar with how it works and don't know how to verify it works. I'll let someone else contribute that in a different PR.
it's the same instructions, just for the .app file
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2378206734)
> That makes sense to me, but i'm not user of the `.app`. I'm unfamiliar with how it works and don't know how to verify it works. I'll let someone else contribute that in a different PR.
it's the same instructions, just for the .app file
💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778028934)
This fails with the same error. It would have also surprised me if a float-cast UB was hit in one code path, but not in an equivalent one.
```
node1 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/chrono.h:212:8: runtime error: inf is outside the range of representable values of type 'long'
#0 0x55cbcf9b4fb3 in std::chrono::duration<long, std::ratio<1l, 1l>> std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1l>>, std::ratio<1l, 1
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778028934)
This fails with the same error. It would have also surprised me if a float-cast UB was hit in one code path, but not in an equivalent one.
```
node1 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/chrono.h:212:8: runtime error: inf is outside the range of representable values of type 'long'
#0 0x55cbcf9b4fb3 in std::chrono::duration<long, std::ratio<1l, 1l>> std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1l>>, std::ratio<1l, 1
...
💬 maflcko commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2378483169)
meta nit: Maybe move the bulk of the pull request description into a new comment. Otherwise it will end up in the merge commit (if this is merged) and everyone will have to scroll through the 24 terminal pages when skimming the commit log.
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2378483169)
meta nit: Maybe move the bulk of the pull request description into a new comment. Otherwise it will end up in the merge commit (if this is merged) and everyone will have to scroll through the 24 terminal pages when skimming the commit log.
💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778105580)
444424b96deef72780f617d1dbab844a2c9d72ab: In case anyone is wondering: https://stackoverflow.com/questions/13142255/what-is-the-difference-between-stdcondition-variablewait-for-and-stdcondit
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778105580)
444424b96deef72780f617d1dbab844a2c9d72ab: In case anyone is wondering: https://stackoverflow.com/questions/13142255/what-is-the-difference-between-stdcondition-variablewait-for-and-stdcondit
💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778162781)
I think that since `waitTipChanged` takes a `current_tip` argument the `m_tip_block != uint256::ZERO` check is no longer needed. Though it seems fine to keep it.
IIRC the reason I added it was in case `waitTipChanged` was called during early node initialization, before `m_tip_block` is set to the genesis block.
That scenario is now handled by `m_tip_block != current_tip`.
In term of how the Template Provider works, it used to rely on this check, but now it just waits for `getTip()` to r
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778162781)
I think that since `waitTipChanged` takes a `current_tip` argument the `m_tip_block != uint256::ZERO` check is no longer needed. Though it seems fine to keep it.
IIRC the reason I added it was in case `waitTipChanged` was called during early node initialization, before `m_tip_block` is set to the genesis block.
That scenario is now handled by `m_tip_block != current_tip`.
In term of how the Template Provider works, it used to rely on this check, but now it just waits for `getTip()` to r
...
💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2378614029)
re-utACK cccc5b19fba6d04e586f59bff5f5b2cdef3dffba
The main change since my last review is in 444424b96deef72780f617d1dbab844a2c9d72ab, which is a nice simplification.
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2378614029)
re-utACK cccc5b19fba6d04e586f59bff5f5b2cdef3dffba
The main change since my last review is in 444424b96deef72780f617d1dbab844a2c9d72ab, which is a nice simplification.
💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2378624163)
> The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files.
Upgrading my review: I tested using the instructions.
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2378624163)
> The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files.
Upgrading my review: I tested using the instructions.
📝 vasild opened a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988)
Currently `CConnman` is a mixture of:
* low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
* higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in `AddrMan`, maintaining the number of inbound and outbound connections, banning of peers, interacting with `PeerManager`.
This PR splits the socket handling into a new class which makes the code more mod
...
(https://github.com/bitcoin/bitcoin/pull/30988)
Currently `CConnman` is a mixture of:
* low level socket handling, e.g. send, recv, poll, bind, listen, connect, and
* higher level logic that is specific to the Bitcoin P2P protocol, e.g. V1/V2 transport, choosing which address to connect to, if we manage to connect mark the address good in `AddrMan`, maintaining the number of inbound and outbound connections, banning of peers, interacting with `PeerManager`.
This PR splits the socket handling into a new class which makes the code more mod
...