💬 Christewart 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-2377685948)
> > > I think it might be good to also include instructions for signing a downloaded `Bitcoin-Qt.app`, as I would think that might be what a lot (majority?) of MacOS users are downloading.
> >
> >
> > I presume this is the GUI? I'm not sure if assuming command line literacy makes sense for that.
>
> Well, as the app does not work at all on macOS without that it seems better to me to at least provide the required steps, rather than nothing at all?
That makes sense to me, but i'm not us
...
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2377685948)
> > > I think it might be good to also include instructions for signing a downloaded `Bitcoin-Qt.app`, as I would think that might be what a lot (majority?) of MacOS users are downloading.
> >
> >
> > I presume this is the GUI? I'm not sure if assuming command line literacy makes sense for that.
>
> Well, as the app does not work at all on macOS without that it seems better to me to at least provide the required steps, rather than nothing at all?
That makes sense to me, but i'm not us
...
💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1777604226)
Unrelated to this pull, the comment is wrong. `Shutdown()` *is* called. So it would be good (in a follow-up), or if you re-touch, to remove the incorrect and confusing comment:
```diff
diff --git a/src/init.cpp b/src/init.cpp
index ebe48dc307..5f786fd4df 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1649,7 +1649,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// As LoadBlockIndex can take several minutes, it's possible the user
//
...
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1777604226)
Unrelated to this pull, the comment is wrong. `Shutdown()` *is* called. So it would be good (in a follow-up), or if you re-touch, to remove the incorrect and confusing comment:
```diff
diff --git a/src/init.cpp b/src/init.cpp
index ebe48dc307..5f786fd4df 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1649,7 +1649,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// As LoadBlockIndex can take several minutes, it's possible the user
//
...
⚠️ ryanofsky opened an issue: "RFC: Multiprocess binaries and packaging options"
(https://github.com/bitcoin/bitcoin/issues/30983)
Issue for discussion about ways multiprocess functionality could be packaged and released.
### Binaries
One goal of the [multiprocess project](https://github.com/bitcoin/bitcoin/issues/28722) has been to provide minimal binaries that only include node, wallet, or gui code, which spawn or connect to other processes as needed to provide other functionality.
The idea implemented in #10102 is to have 3 binaries:
- **`bitcoin-node`** - contains node code and libraries (leveldb), and no wa
...
(https://github.com/bitcoin/bitcoin/issues/30983)
Issue for discussion about ways multiprocess functionality could be packaged and released.
### Binaries
One goal of the [multiprocess project](https://github.com/bitcoin/bitcoin/issues/28722) has been to provide minimal binaries that only include node, wallet, or gui code, which spawn or connect to other processes as needed to provide other functionality.
The idea implemented in #10102 is to have 3 binaries:
- **`bitcoin-node`** - contains node code and libraries (leveldb), and no wa
...
💬 maflcko commented on pull request "ci: Inline PACKAGE_MANAGER_INSTALL":
(https://github.com/bitcoin/bitcoin/pull/30974#issuecomment-2377728205)
(rebased)
(https://github.com/bitcoin/bitcoin/pull/30974#issuecomment-2377728205)
(rebased)
👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2332196385)
Code review ACK fa24d8dbddebd61b9c418bd6773c333ad72bed67. Fixed wait_for overflow bug since last review
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2332196385)
Code review ACK fa24d8dbddebd61b9c418bd6773c333ad72bed67. Fixed wait_for overflow bug since last review
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777641007)
In commit "refactor: Add missing GUARDED_BY(m_tip_block_mutex)" (fa47d893acdd18132204810dd95880ab21abf415)
It seems like it would be a little better to make this change in the next commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439) instead of this one so this code is not changing more than once.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777641007)
In commit "refactor: Add missing GUARDED_BY(m_tip_block_mutex)" (fa47d893acdd18132204810dd95880ab21abf415)
It seems like it would be a little better to make this change in the next commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439) instead of this one so this code is not changing more than once.
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777642365)
In commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439)
Might be better to avoid the 100 year hack by switching to wait_until
```diff
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -940,10 +940,10 @@ public:
BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
- if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777642365)
In commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439)
Might be better to avoid the 100 year hack by switching to wait_until
```diff
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -940,10 +940,10 @@ public:
BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
- if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in
...
💬 ryanofsky commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2377800457)
> For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term.
Update: currently working on getting mingw32 build working, and I created issue #30983 to lay out different multiprocess release options.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2377800457)
> For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term.
Update: currently working on getting mingw32 build working, and I created issue #30983 to lay out different multiprocess release options.
💬 maflcko commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377843249)
> Wouldn't a developer looking to fuzz test set `-DBUILD_FOR_FUZZING=ON`?
Most likely yes, but it isn't enforced. For example, it is possible to compile the fuzz binary without `DBUILD_FOR_FUZZING` and execute it. Support for that is also added to the `fuzz/test_runner.py` script. (This is also what the failing CIs were using to call the fuzz binary).
> Would future harnesses relying on FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION also do an early return?
I'd say it depends on the fuzz targ
...
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377843249)
> Wouldn't a developer looking to fuzz test set `-DBUILD_FOR_FUZZING=ON`?
Most likely yes, but it isn't enforced. For example, it is possible to compile the fuzz binary without `DBUILD_FOR_FUZZING` and execute it. Support for that is also added to the `fuzz/test_runner.py` script. (This is also what the failing CIs were using to call the fuzz binary).
> Would future harnesses relying on FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION also do an early return?
I'd say it depends on the fuzz targ
...
💬 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
...