💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1913161408)
> Could you clarify what deprecation will actually mean for 27.0, and then the future steps?
I would have thought that for 27.x you'd just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0...
> For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial
...
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1913161408)
> Could you clarify what deprecation will actually mean for 27.0, and then the future steps?
I would have thought that for 27.x you'd just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0...
> For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial
...
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1468498323)
@mzumsande, @sr-gi, I played with @sr-gi's idea on [this branch](https://github.com/stratospher/bitcoin/commit/276b95d766cd11d65d563805d52beace8dd32744) and wondering about 2 things:
1. can deadlocks happen? i noticed places in the test where `_send_lock` was acquired and then `p2p_lock` (and deadlocks didn't happen). am i missing something?
1. MainThread acquires `_send_lock`
2. NetworkThread acquires `p2p_lock`
3. NetworkThread would wait for MainThread to release `_send_lock`
4. Ma
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1468498323)
@mzumsande, @sr-gi, I played with @sr-gi's idea on [this branch](https://github.com/stratospher/bitcoin/commit/276b95d766cd11d65d563805d52beace8dd32744) and wondering about 2 things:
1. can deadlocks happen? i noticed places in the test where `_send_lock` was acquired and then `p2p_lock` (and deadlocks didn't happen). am i missing something?
1. MainThread acquires `_send_lock`
2. NetworkThread acquires `p2p_lock`
3. NetworkThread would wait for MainThread to release `_send_lock`
4. Ma
...
💬 dooglus commented on issue "New crash in v26.0":
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913182621)
@mzumsande wrote:
> Looks like the crash happens within libQt5Core.so.5, for which there are no debug symbols available.
OK. I will install Qt5 packages with debug symbols and wait for it to crash again.
> I tried for a while to reproduce the crash but didn't manage. Is there something special or "unusual" about your setup, for example are you loading a large number of wallets, or are some of the wallets very large?
No, I don't think so. I only load 3 or 4 wallets on startup. Yesterd
...
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913182621)
@mzumsande wrote:
> Looks like the crash happens within libQt5Core.so.5, for which there are no debug symbols available.
OK. I will install Qt5 packages with debug symbols and wait for it to crash again.
> I tried for a while to reproduce the crash but didn't manage. Is there something special or "unusual" about your setup, for example are you loading a large number of wallets, or are some of the wallets very large?
No, I don't think so. I only load 3 or 4 wallets on startup. Yesterd
...
💬 1440000bytes commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#discussion_r1468503986)
Concept ACK because default policy is unchanged.
> I don't care if this gets merged, personally **I don't see any benefit of having this option** (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to https://github.com/bitcoin/bitcoin/pull/28217).
It might be useful for testing and a few other use cases.
> Concept ACK, it's great to give users the option of filtering out potentially abusive txs
The only potential abuse
...
(https://github.com/bitcoin/bitcoin/pull/29309#discussion_r1468503986)
Concept ACK because default policy is unchanged.
> I don't care if this gets merged, personally **I don't see any benefit of having this option** (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to https://github.com/bitcoin/bitcoin/pull/28217).
It might be useful for testing and a few other use cases.
> Concept ACK, it's great to give users the option of filtering out potentially abusive txs
The only potential abuse
...
👋 russeree's pull request is ready for review: "redeclare nChainTx to use uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29331)
(https://github.com/bitcoin/bitcoin/pull/29331)
💬 russeree commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1913242734)
Would this be a hard fork because it would be loosening the conditions for validation?
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1913242734)
Would this be a hard fork because it would be loosening the conditions for validation?
💬 luke-jr commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274)
This still isn't a wallet option...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274)
This still isn't a wallet option...
📝 hebasto opened a pull request: "Add missed `config/bitcoin-config.h` header"
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
💬 hebasto commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584127)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Would it make sense to construct the `group_pos` variable directly from the fuzzer? I think you only need to populate its `m_weight` and `m_effective_value`. As you don't use anything but `group_pos`, `target`, and `change_target` in the actual test, all fuzz information you use to construct other things is effectively wasted.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584127)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Would it make sense to construct the `group_pos` variable directly from the fuzzer? I think you only need to populate its `m_weight` and `m_effective_value`. As you don't use anything but `group_pos`, `target`, and `change_target` in the actual test, all fuzz information you use to construct other things is effectively wasted.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468544154)
In commit "fuzz: Add CoinGrinder fuzz target"
`coin_params.m_min_change_target` is left at 0 here. Perhaps leave a comment about why that is ok?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468544154)
In commit "fuzz: Add CoinGrinder fuzz target"
`coin_params.m_min_change_target` is left at 0 here. Perhaps leave a comment about why that is ok?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584560)
In commit "wallet: Add CoinGrinder coin selection algorithm":
SRD is not used in this test, so I think you can construct `change_target` from fuzz data directly.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584560)
In commit "wallet: Add CoinGrinder coin selection algorithm":
SRD is not used in this test, so I think you can construct `change_target` from fuzz data directly.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513733)
In commit "coinselection: Add CoinGrinder algorithm"
Not as much in this commit yet, but in the PR overall, almost everywhere the sum `selection_target + change_target` is used. Maybe it makes sense to have a `const auto total_target = selection_target + change_target`, and use `total_target` everywhere?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513733)
In commit "coinselection: Add CoinGrinder algorithm"
Not as much in this commit yet, but in the PR overall, almost everywhere the sum `selection_target + change_target` is used. Maybe it makes sense to have a `const auto total_target = selection_target + change_target`, and use `total_target` everywhere?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584961)
In commit "wallet: Add CoinGrinder coin selection algorithm":
I think it would be better if you'd also construct `max_weight` from the fuzz data (and then take it into account in the brute force loop below).
As an alternative, run the brute force loop first (without any weight maximum restriction), and then run CG twice, once with a (fuzz-constructed) `max_weight` >= the brute force best solution (and check that CG finds it) and once with a (fuzz-constructed) `max_weight` < the brute force
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584961)
In commit "wallet: Add CoinGrinder coin selection algorithm":
I think it would be better if you'd also construct `max_weight` from the fuzz data (and then take it into account in the brute force loop below).
As an alternative, run the brute force loop first (without any weight maximum restriction), and then run CG twice, once with a (fuzz-constructed) `max_weight` >= the brute force best solution (and check that CG finds it) and once with a (fuzz-constructed) `max_weight` < the brute force
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468517854)
In commit "coinselection: Add CoinGrinder algorithm"
I don't believe it's actually possible to not have a solution at this point, unless the maximum weight is exceeded because the two ways of achieving that are:
* Not enough funds (but that is checked early in the function)
* A solution exists, but wasn't found due to computation limits. However, with a UTXO set of $n$ elements/groups, *a* solution should always be found within $n$ iterations (and I think the number of iterations should be
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468517854)
In commit "coinselection: Add CoinGrinder algorithm"
I don't believe it's actually possible to not have a solution at this point, unless the maximum weight is exceeded because the two ways of achieving that are:
* Not enough funds (but that is checked early in the function)
* A solution exists, but wasn't found due to computation limits. However, with a UTXO set of $n$ elements/groups, *a* solution should always be found within $n$ iterations (and I think the number of iterations should be
...