💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1777703574)
This changes the error handling if the block tree db is corrupted a bit worse as a side effect. Before, we'd get the advice "Please restart with -reindex or -reindex-chainstate to recover." on the console, and, if using the GUI, the option to reindex with by clicking a button. Now we just get a "Error opening block database" error on both without any advice how to fix it.
I'm not sure if the GUI one-click reindex option is totally essential, but in my opinion we should at least keep the advic
...
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1777703574)
This changes the error handling if the block tree db is corrupted a bit worse as a side effect. Before, we'd get the advice "Please restart with -reindex or -reindex-chainstate to recover." on the console, and, if using the GUI, the option to reindex with by clicking a button. Now we just get a "Error opening block database" error on both without any advice how to fix it.
I'm not sure if the GUI one-click reindex option is totally essential, but in my opinion we should at least keep the advic
...
🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2332298896)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2332298896)
Concept ACK
💬 instagibbs commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377936088)
@sdaftuar I think the same reasoning I gave is just being applied recursively, which is fine :+1:
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377936088)
@sdaftuar I think the same reasoning I gave is just being applied recursively, which is fine :+1:
🤔 hodlinator reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2332231261)
Concept ACK
Feels like *p2p_orphan_handling.py*/`test_same_txid_orphan()` might benefit from some augmented checking using this new `getorphantxs`. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2332231261)
Concept ACK
Feels like *p2p_orphan_handling.py*/`test_same_txid_orphan()` might benefit from some augmented checking using this new `getorphantxs`. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.
💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777666525)
My guess is that this weird indentation style is an artifact of refactorings not wanting to re-indent old lines. Should not apply to new code.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777666525)
My guess is that this weird indentation style is an artifact of refactorings not wanting to re-indent old lines. Should not apply to new code.
💬 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.