💬 epiccurious commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1923932584)
Tested ACK.

>not actual tests that can be executed directly
Thanks for the clarification.

Certain tests take longer to run, to be expected with the added sub-tests.
```
TEST | STATUS | DURATION

feature_block.py | ✓ Passed | 197 s
feature_maxuploadtarget.py | ✓ Passed | 59 s
p2p_ibd_stalling.py | ✓ Passed | 59 s
p2p_ibd_stalling.py --v2transport | ✓ Passed | 58 s
p2p_invalid_messages.py | ✓ Passed
...

💬 stickies-v commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1923969918)
Force pushed to rebase, added a commit to bump compilers of failing CI tasks (see [OP](https://github.com/bitcoin/bitcoin/pull/28687#issue-1952153535)), not too sure what I'm doing here though.

🤔 murchandamus reviewed a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1859460255)
Concept ACK

💬 murchandamus commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476168453)
Given that the PR describes the setting of the flag as a special case for blank wallets, I’d be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true here.

```diff
- success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
+ if(local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
+ local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+ success = true;
+ }
+
i
...

⚠️ Thrillseekr opened an issue: "How to get started with Contribution"
(https://github.com/bitcoin-core/gui/issues/791)
Hello @gregwebs @ldenman @casey @eklitzke @rion,
I'm Darshan Jain, interested in contributing to your organization Bitcoin Core.

Could you please help me in guiding how and where to get started.

My interests and skills lies in domain
C++, Object Oriented Programming Techniques, Data Structures and Algorithms

💬 hebasto commented on issue "How to get started with Contribution":
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1924103289)
@Thrillseekr

You are much welcome!

Please consider https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#getting-started.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1924145169)
If i re-review this would anyone else review as well? ACKed it multiple times in the past but it didn't get merged because we'd need another person.

It's been a while, i need to review it from scratch so i'm trying to probe if that's worth spending the time.

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1476232308)
The following patch
```diff
--- a/contrib/guix/manifest.scm
+++ b/contrib/guix/manifest.scm
@@ -108,6 +108,17 @@ desirable for building Bitcoin Core release binaries."
base-libc
base-gcc))

+
+(define-public mingw-w64-x86_64-winpthreads-ucrt
+ (package
+ (inherit mingw-w64-x86_64-winpthreads)
+ (arguments
+ (substitute-keyword-arguments (package-arguments mingw-w64-x86_64-winpthreads)
+ ((#:configure-flags flags)
...

🤔 jonatack reviewed a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217#pullrequestreview-1859525850)
ACK 8672721deb06e66854a085c9994e99c99160b8a1

---

> ACK.

@theDavidCoen In your review https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914214959, per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review (and to be counted by DrahtBot and the merge script), your ACK would need to be followed by the commit hash. See above here or https://github.com/bitcoin/bitcoin/pull/28217#pullrequestreview-1842509290 for examples.

💬 jonatack commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#discussion_r1476207219)
Only if you need to repush, so as not to invalidate the current review ACKs, in commit 8672721deb06e66854a085c9994e99c99160b8a1 perhaps consider the following diff.

```diff
-P2P and network changes
------------------------
+Updated settings
+----------------

-- Changing the default setting of -permitbaremultisig to false. Non-P2SH multisig transactions will no longer be relayed by default. (#28217)
\ No newline at end of file
+- The default value of the -permitbaremultisig configura
...

💬 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??

💬 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
...

💬 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

📝 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
...

💬 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

💬 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?

💬 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 `>=`.

💬 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.

💬 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}`

💬 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?