📝 hebasto opened a pull request: "build: Patch Qt to handle minimum macOS version properly"
(https://github.com/bitcoin/bitcoin/pull/28851)
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVI
...
(https://github.com/bitcoin/bitcoin/pull/28851)
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVI
...
💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806576609)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/28851.
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806576609)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/28851.
✅ hebasto closed a pull request: "build: Patch Qt to handle minimum macOS version properly"
(https://github.com/bitcoin/bitcoin/pull/28775)
(https://github.com/bitcoin/bitcoin/pull/28775)
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390055024)
These two things are not technically equivalent, because `send` creates a change output here, which results in intermittent failures for the rest of the test. Is there any way to tell `send` not to create a change output?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390055024)
These two things are not technically equivalent, because `send` creates a change output here, which results in intermittent failures for the rest of the test. Is there any way to tell `send` not to create a change output?
💬 ajtowns commented on pull request "[refactor] Remove BlockAssembler m_mempool member":
(https://github.com/bitcoin/bitcoin/pull/28843#discussion_r1390058040)
Why aren't you just changing `addPackageTxs` to use `m_mempool` directly? Seems simpler, and doesn't change the public api. Something like https://github.com/ajtowns/bitcoin/commit/944d8bc87690389de9bd51a6ea8a3223d10e4002#diff-352b412a72c7d069b77b786164c6681ee324ff0d8e61f01d48057e297db991fa
(https://github.com/bitcoin/bitcoin/pull/28843#discussion_r1390058040)
Why aren't you just changing `addPackageTxs` to use `m_mempool` directly? Seems simpler, and doesn't change the public api. Something like https://github.com/ajtowns/bitcoin/commit/944d8bc87690389de9bd51a6ea8a3223d10e4002#diff-352b412a72c7d069b77b786164c6681ee324ff0d8e61f01d48057e297db991fa
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390071466)
I think that `TxStateBlockConflicted` and `TxStateMempoolConflicted` are a bit more clear, because with something like `TxStateUnconfirmedConflict` instead of `TxStateMempoolConflicted`, it is unclear whether the conflict is in the mempool, or just hasn't been broadcasted yet. Additionally, I felt that it was more uniform with the other transaction states to be describing the state of the transaction instead of the state of a conflict. Eg. `TxStateBlockConflicted` indicates that the transaction
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390071466)
I think that `TxStateBlockConflicted` and `TxStateMempoolConflicted` are a bit more clear, because with something like `TxStateUnconfirmedConflict` instead of `TxStateMempoolConflicted`, it is unclear whether the conflict is in the mempool, or just hasn't been broadcasted yet. Additionally, I felt that it was more uniform with the other transaction states to be describing the state of the transaction instead of the state of a conflict. Eg. `TxStateBlockConflicted` indicates that the transaction
...
🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1725862532)
Updated per feedback, thanks for the review @vasild!
The node will now request blocks within the limited peers threshold when the block download window contemplates them and not only when the previous blocks arrived or are in-flight. Have added test coverage exercising this behavior and verifying that the node can get synced even when the blocks arrived out of order.
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1725862532)
Updated per feedback, thanks for the review @vasild!
The node will now request blocks within the limited peers threshold when the block download window contemplates them and not only when the previous blocks arrived or are in-flight. Have added test coverage exercising this behavior and verifying that the node can get synced even when the blocks arrived out of order.
💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390079370)
Removed the unconditional wait on the last update :).
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390079370)
Removed the unconditional wait on the last update :).
💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390075262)
Ended up doing it in a slightly different way. Using `setnetworkactive` to isolate the full node from the other two. I think that is clearer in this way.
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390075262)
Ended up doing it in a slightly different way. Using `setnetworkactive` to isolate the full node from the other two. I think that is clearer in this way.
💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390079461)
Added `NODE_NETWORK_LIMITED_MIN_BLOCKS` on the last update.
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1390079461)
Added `NODE_NETWORK_LIMITED_MIN_BLOCKS` on the last update.
💬 Abuchtela commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1806630463)
`Rebase`
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1806630463)
`Rebase`
💬 theStack commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1390101144)
One small drawback of the current way of inspecting `extra_args` for the v2transport option is that it doesn't work if the argument is specified in a slightly different format with same meaning. For example, `-v2transport=0`/`-v2transport=1` could also be passed as `-nov2transport`/`-v2transport`, `-nov2transport=1`/`-nov2transport=0` (admittedly, no sane person would use the last one though). It might be worth it to consider that in a follow-up, e.g. by enforcing with an assertion that argument
...
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1390101144)
One small drawback of the current way of inspecting `extra_args` for the v2transport option is that it doesn't work if the argument is specified in a slightly different format with same meaning. For example, `-v2transport=0`/`-v2transport=1` could also be passed as `-nov2transport`/`-v2transport`, `-nov2transport=1`/`-nov2transport=0` (admittedly, no sane person would use the last one though). It might be worth it to consider that in a follow-up, e.g. by enforcing with an assertion that argument
...
👍 theStack approved a pull request: "test: Make existing functional tests compatible with --v2transport"
(https://github.com/bitcoin/bitcoin/pull/28805#pullrequestreview-1725906389)
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
(https://github.com/bitcoin/bitcoin/pull/28805#pullrequestreview-1725906389)
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
💬 stratospher commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1390105880)
> "if no extra_args are passed, restart with the ones that were initially specified (self.extra_args)"
oh interesting! so https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1388909333 is out of scope for this PR.
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1390105880)
> "if no extra_args are passed, restart with the ones that were initially specified (self.extra_args)"
oh interesting! so https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1388909333 is out of scope for this PR.
🤔 stratospher reviewed a pull request: "test: Make existing functional tests compatible with --v2transport"
(https://github.com/bitcoin/bitcoin/pull/28805#pullrequestreview-1725912169)
ACK 35fb993.
(https://github.com/bitcoin/bitcoin/pull/28805#pullrequestreview-1725912169)
ACK 35fb993.
💬 sipa commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1806662743)
utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1806662743)
utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128297)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128297)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128370)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128370)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128432)
I've added a comment.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128432)
I've added a comment.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128566)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128566)
Fixed