š¬ Thrillseekr commented on issue "How to get started with Contribution":
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924171935)
Thanks @hebasto ,
I've gone through the doc you provided and understood that "Review" and "Testing" are the ways to start with.
But along with these, I've to go through the build process and codebase so that I can have an overview of what going on.
How should I proceed now??
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924171935)
Thanks @hebasto ,
I've gone through the doc you provided and understood that "Review" and "Testing" are the ways to start with.
But along with these, I've to go through the build process and codebase so that I can have an overview of what going on.
How should I proceed now??
š¬ hebasto commented on issue "How to get started with Contribution":
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924180051)
https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#getting-started:
> There are many open issues of varying difficulty waiting to be fixed. If you're looking for somewhere to start contributing, check out the [good first issue](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) list or changes that are [up for grabs](https://github.com/bitcoin/bitcoin/issues?utf8=%E2%9C%93&q=label%3A%22Up+for+grabs%22). Some of them might no longer be
...
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924180051)
https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#getting-started:
> There are many open issues of varying difficulty waiting to be fixed. If you're looking for somewhere to start contributing, check out the [good first issue](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) list or changes that are [up for grabs](https://github.com/bitcoin/bitcoin/issues?utf8=%E2%9C%93&q=label%3A%22Up+for+grabs%22). Some of them might no longer be
...
š¬ jonatack commented on issue "How to get started with Contribution":
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924194007)
@Thrillseekr I also suggest the following resource: https://jonatack.github.io/articles
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924194007)
@Thrillseekr I also suggest the following resource: https://jonatack.github.io/articles
š ryanofsky opened a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370)
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them.
Note: This cha
...
(https://github.com/bitcoin/bitcoin/pull/29370)
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them.
Note: This cha
...
š¬ ryanofsky commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1924198876)
Attempting to get rid of the fake values in #29370
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1924198876)
Attempting to get rid of the fake values in #29370
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476281814)
Maybe instead of magic number, calculate/specify the parent feerate and make `maxfeerate` a little bit less?
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476281814)
Maybe instead of magic number, calculate/specify the parent feerate and make `maxfeerate` a little bit less?
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476284777)
Similarly, maybe specify `maxburnamount` as == the output amount? Imo a test should fail if I switch the check from `>` to `>=`.
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476284777)
Similarly, maybe specify `maxburnamount` as == the output amount? Imo a test should fail if I switch the check from `>` to `>=`.
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476265685)
nit: I think calling this `m_client_maxfeerate` would make more sense. Don't think "sane" tells us very much, and it'd be good to specify that this comes from the client. Generally I don't think "sanity check" is the right term for these, just "check," as the user can provide a strange or no value and we don't judge whether it makes sense or not.
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476265685)
nit: I think calling this `m_client_maxfeerate` would make more sense. Don't think "sane" tells us very much, and it'd be good to specify that this comes from the client. Generally I don't think "sanity check" is the right term for these, just "check," as the user can provide a strange or no value and we don't judge whether it makes sense or not.
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476268224)
Probably good time to make a `DEFAULT_MAX_BURN_AMOUNT{0}`
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476268224)
Probably good time to make a `DEFAULT_MAX_BURN_AMOUNT{0}`
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476276883)
I can see why we'd check individual feerate instead of chunk feerate, since we don't currently check whether there's a real CPFP happening and which transactions are in the chunk containing a CPFP. Could add a TODO item explaining this?
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476276883)
I can see why we'd check individual feerate instead of chunk feerate, since we don't currently check whether there's a real CPFP happening and which transactions are in the chunk containing a CPFP. Could add a TODO item explaining this?
š¬ glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476287480)
random newline?
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476287480)
random newline?
š¬ maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1924238244)
Yes, either use a newer GCC, or remove the `value_type` type, as in C++20 it is expected to be derived from `element_type` via `std::remove_cv_t<element_type>`.
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1924238244)
Yes, either use a newer GCC, or remove the `value_type` type, as in C++20 it is expected to be derived from `element_type` via `std::remove_cv_t<element_type>`.
š ryanofsky approved a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1859682311)
Code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1859682311)
Code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7
š¬ ryanofsky commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476295451)
> Iād be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true
Just to be clear, these two conditions should be exactly equivalent due to the `success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);` line so should be a style change, not a change in behavior. I agree the suggestion looks a little better, but the style of dealing with the `success` variable seems here to follow a particular pattern, so would leave it up to
...
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476295451)
> Iād be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true
Just to be clear, these two conditions should be exactly equivalent due to the `success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);` line so should be a style change, not a change in behavior. I agree the suggestion looks a little better, but the style of dealing with the `success` variable seems here to follow a particular pattern, so would leave it up to
...
š Christewart opened a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371)
This PR adds a `leaf_version` parameter to `taproot_tree_helper()`. Previously the leaf version was hard coded, because we only currently support 1 leaf version.
Proposed soft forks such as #29221 require new leaf versions to be allocated. This PR allows the test framework to accomodate new leaf versions in the test framework.
(https://github.com/bitcoin/bitcoin/pull/29371)
This PR adds a `leaf_version` parameter to `taproot_tree_helper()`. Previously the leaf version was hard coded, because we only currently support 1 leaf version.
Proposed soft forks such as #29221 require new leaf versions to be allocated. This PR allows the test framework to accomodate new leaf versions in the test framework.
š¬ maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1924254747)
> Backwards compatibility code in LoadBlockIndex to override `nTx == 1` values set by previous code when they are fake (when ` (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`)
Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1924254747)
> Backwards compatibility code in LoadBlockIndex to override `nTx == 1` values set by previous code when they are fake (when ` (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`)
Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.
š¬ achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924257982)
> Might be worth revisiting this now that the cache is much simpler. The signing stuff could be a follow-up PR, but if it's simple enough it would be really nice to include it here. Otherwise, someone using migrate wallet could still hit a wall even with the cache when trying to sign a transaction.
I don't think it's possible to do caching for signing in a sane way that doesn't involve rewriting how we manage keys. The crux of the issue is that we will sign for things even if the wallet has o
...
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924257982)
> Might be worth revisiting this now that the cache is much simpler. The signing stuff could be a follow-up PR, but if it's simple enough it would be really nice to include it here. Otherwise, someone using migrate wallet could still hit a wall even with the cache when trying to sign a transaction.
I don't think it's possible to do caching for signing in a sane way that doesn't involve rewriting how we manage keys. The crux of the issue is that we will sign for things even if the wallet has o
...
š¬ epiccurious commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#issuecomment-1924263560)
Tested ACK.
(https://github.com/bitcoin/bitcoin/pull/29367#issuecomment-1924263560)
Tested ACK.
š¬ glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476296848)
Instead of the 021ecfea062151bce0d67bf1575fe7e5e1ee22db approach, I actually think `must contain between 2 and MAX_PACKAGE_COUNT transactions` would be a more helpful interface for users
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476296848)
Instead of the 021ecfea062151bce0d67bf1575fe7e5e1ee22db approach, I actually think `must contain between 2 and MAX_PACKAGE_COUNT transactions` would be a more helpful interface for users
š¬ glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476310366)
I don't really understand why b2fb55cabc76d842c58b51ff9c64126e6639d1bb is better - I'd interpret this as an array of literal strings "rawtx1", "rawtx2" ?
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476310366)
I don't really understand why b2fb55cabc76d842c58b51ff9c64126e6639d1bb is better - I'd interpret this as an array of literal strings "rawtx1", "rawtx2" ?
š maflcko converted_to_draft a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369)
Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as `std::span`.
Fix that.
(https://github.com/bitcoin/bitcoin/pull/29369)
Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as `std::span`.
Fix that.