💬 hebasto commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1825031652)
I wouldn't expect that all packages have the same names across Ubuntu/Debian and Homebrew repositories.
I agree that a macOS-specific section can be added here, but this is not a goal of this PR.
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1825031652)
I wouldn't expect that all packages have the same names across Ubuntu/Debian and Homebrew repositories.
I agree that a macOS-specific section can be added here, but this is not a goal of this PR.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825032783)
Ah, it's because at this point, the `Apply()` function doesn't use the ancestors anyway. Agreed that this is pretty ugly -- I'll try to fix.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825032783)
Ah, it's because at this point, the `Apply()` function doesn't use the ancestors anyway. Agreed that this is pretty ugly -- I'll try to fix.
💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2450633698)
I added commits to drop `getTransactionsUpdated()` and `testBlockValidity()` as well.
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2450633698)
I added commits to drop `getTransactionsUpdated()` and `testBlockValidity()` as well.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996376)
Thanks, great suggestion. I’ve adopted that change.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996376)
Thanks, great suggestion. I’ve adopted that change.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996009)
Thanks, great, taken!
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1824996009)
Thanks, great, taken!
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825015274)
Yes, this tests that BnB will fail to find the best solution when the attempts exceed the `TOTAL_TRIES` limit of 100,000. This test follows the same approach but is slightly different than the one in the original tests. I wanted to replace the old test because I found it too difficult to parse, but I came up with my own way of testing it. I have expanded the explanation of what is going on here, and how I created this artificial example that manages to exhaust the limit with only 18 UTXOs.—To be
...
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825015274)
Yes, this tests that BnB will fail to find the best solution when the attempts exceed the `TOTAL_TRIES` limit of 100,000. This test follows the same approach but is slightly different than the one in the original tests. I wanted to replace the old test because I found it too difficult to parse, but I came up with my own way of testing it. I have expanded the explanation of what is going on here, and how I created this artificial example that manages to exhaust the limit with only 18 UTXOs.—To be
...
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825044678)
I added a longer explanation to this test. Luckily 18 is not generally the limeit, it’s just the limit in this artificially crafted case where all of our UTXOs have very similar same amounts and we have to pick eight of them to meet the target. Please let me know if it makes sense now! :)
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825044678)
I added a longer explanation to this test. Luckily 18 is not generally the limeit, it’s just the limit in this artificially crafted case where all of our UTXOs have very similar same amounts and we have to pick eight of them to meet the target. Please let me know if it makes sense now! :)
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825000480)
Yeah, as explained, the intention is to rewrite all coinselection tests with transactions that could be encountered in the wild rather than the artificial zero-fee transactions the old tests are using.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825000480)
Yeah, as explained, the intention is to rewrite all coinselection tests with transactions that could be encountered in the wild rather than the artificial zero-fee transactions the old tests are using.
🤔 murchandamus reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2408805041)
Thanks @furszy, @nymius, @jasonribble, and @vicariousdrama for the review!
I updated the commit message names as suggested, adopted all of your proposed changes as indicated, and added more explanation to the test covering the attempt limit.
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2408805041)
Thanks @furszy, @nymius, @jasonribble, and @vicariousdrama for the review!
I updated the commit message names as suggested, adopted all of your proposed changes as indicated, and added more explanation to the test covering the attempt limit.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825047813)
Thanks, I’ll make a note to update the benchmark as well.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1825047813)
Thanks, I’ll make a note to update the benchmark as well.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2450644019)
> Also, do you think is worth to add these conventions to the `CONTRIBUTIONS.md` file?
I don’t know! It would be a good thing to ask in the IRC channel or at the weekly meeting. :)
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2450644019)
> Also, do you think is worth to add these conventions to the `CONTRIBUTIONS.md` file?
I don’t know! It would be a good thing to ask in the IRC channel or at the weekly meeting. :)
💬 ryanofsky commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450648747)
Maybe a flexible way to support this would be to add wait options to the `createNewBlock()` method so instead of creating a new block template right away, it could optionally wait for conditions to be reached. Those conditions could be set by the client or determined by the node.
Something like:
<details><summary>diff</summary>
<p>
```diff
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -90,9 +90,13 @@ public:
*
* @param[in] script_pub_key the coinbase o
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450648747)
Maybe a flexible way to support this would be to add wait options to the `createNewBlock()` method so instead of creating a new block template right away, it could optionally wait for conditions to be reached. Those conditions could be set by the client or determined by the node.
Something like:
<details><summary>diff</summary>
<p>
```diff
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -90,9 +90,13 @@ public:
*
* @param[in] script_pub_key the coinbase o
...
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825058258)
ok so I think I was a bit wrong, these values were being used and would "blind" us potentially from catching the "BUG! Mempool ancestors or descendants were underestimated" error.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825058258)
ok so I think I was a bit wrong, these values were being used and would "blind" us potentially from catching the "BUG! Mempool ancestors or descendants were underestimated" error.
💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2450652369)
> An easy first fix would be to downgrade InitError to InitWarning, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
Do we also need to explicitly remote the entry from settings.json as well, to make sure it doesn't appear at every start after that. Or does this happen automatically in re-saving it somehow?
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2450652369)
> An easy first fix would be to downgrade InitError to InitWarning, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
Do we also need to explicitly remote the entry from settings.json as well, to make sure it doesn't appear at every start after that. Or does this happen automatically in re-saving it somehow?
⚠️ m3dwards opened an issue: "CI: Improve documentation around replicating CI locally"
(https://github.com/bitcoin/bitcoin/issues/31199)
I've received some feedback that replicating CI jobs locally isn't straightforward. Perhaps the documentation can be improved in this area?
CC @maflcko
(https://github.com/bitcoin/bitcoin/issues/31199)
I've received some feedback that replicating CI jobs locally isn't straightforward. Perhaps the documentation can be improved in this area?
CC @maflcko
⚠️ m3dwards opened an issue: "CI: Make failure message easier to spot"
(https://github.com/bitcoin/bitcoin/issues/31200)
Sometimes during a CI failure the actual error message can be buried in a wall of log output and not immediately clear. Ideally it should be obvious why a CI job has failed.
CC @maflcko
(https://github.com/bitcoin/bitcoin/issues/31200)
Sometimes during a CI failure the actual error message can be buried in a wall of log output and not immediately clear. Ideally it should be obvious why a CI job has failed.
CC @maflcko
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450797714)
@ryanofsky that approach makes sense to me. In that case perhaps `waitFeesChanged()` won't be needed as its own method #31003? That has a nice side-effect in the current implementation of letting us return the last generated block template (I hope eventually we can get the next-block-fees without generating templates at all).
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450797714)
@ryanofsky that approach makes sense to me. In that case perhaps `waitFeesChanged()` won't be needed as its own method #31003? That has a nice side-effect in the current implementation of letting us return the last generated block template (I hope eventually we can get the next-block-fees without generating templates at all).
💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825202486)
Yes, as far as i can tell it's an artifact of switching between protocols. I should remove it. In fact i think `g_mapport_enabled` can be removed as well as we can just use `g_mapport_interrupt` to stop it.
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1825202486)
Yes, as far as i can tell it's an artifact of switching between protocols. I should remove it. In fact i think `g_mapport_enabled` can be removed as well as we can just use `g_mapport_interrupt` to stop it.
📝 l0rinc converted_to_draft a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144)
The recently merged [XOR obfuscation](https://github.com/bitcoin/bitcoin/pull/28207) is a non-trivial part of serialization, encountered during IBD and `reindex`.
In this PR, I've added a new test (comparing randomized inputs against a trivial implementation), updated the benchmark to validate more realistic scenarios (collected every call of `util::Xor` for the first 400k blocks and modelled the benchmark to have the same distribution of data) and optimized the Xor function to do the work in
...
(https://github.com/bitcoin/bitcoin/pull/31144)
The recently merged [XOR obfuscation](https://github.com/bitcoin/bitcoin/pull/28207) is a non-trivial part of serialization, encountered during IBD and `reindex`.
In this PR, I've added a new test (comparing randomized inputs against a trivial implementation), updated the benchmark to validate more realistic scenarios (collected every call of `util::Xor` for the first 400k blocks and modelled the benchmark to have the same distribution of data) and optimized the Xor function to do the work in
...
👍 tdb3 approved a pull request: "init: warn, don't error, when '-upnp' is set"
(https://github.com/bitcoin/bitcoin/pull/31198#pullrequestreview-2409213226)
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
Sanity check below.
On master (f07a533dfcb172321972e5afb3b38a4bd24edb87), prevents start:
PR branch (a1b3ccae4be82297fd20f5be15a03eeb477507d0), doesn't:
(https://github.com/bitcoin/bitcoin/pull/31198#pullrequestreview-2409213226)
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
Sanity check below.
On master (f07a533dfcb172321972e5afb3b38a4bd24edb87), prevents start:
PR branch (a1b3ccae4be82297fd20f5be15a03eeb477507d0), doesn't:
