🤔 hernanmarino reviewed a pull request: "doc: fixup help output for -upnp and -natpmp"
(https://github.com/bitcoin/bitcoin/pull/28874#pullrequestreview-1998681170)
ACK 92f88a962908c49dde99c03a4608e63e4a6eec71

💬 hernanmarino commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2052697672)
Concept ACK . I 'd really like to see this merged

👍 hernanmarino approved a pull request: "validation: don't clear cache on periodic flush: >2x block connection speed"
(https://github.com/bitcoin/bitcoin/pull/28233#pullrequestreview-1998740372)
Concept ACK. I also agree with the criteria in the code for setting the `empty_cache` variable to true

📝 cryptoquick opened a pull request: "Update unix build instructions."
(https://github.com/bitcoin/bitcoin/pull/29864)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 okorye commented on issue "Error unlocking wallet: some keys decrypt but not all. your wallet file may be corrupt. ":
(https://github.com/bitcoin/bitcoin/issues/29789#issuecomment-2052715973)
```
https://t.me/Cryptocom_DeFiWallet/236016<?xml version="1.0" encoding="UTF-8"?>
<feed xml:lang="en-US" xmlns="http://www.w3.org/2005/Atom">`0xe4efdd2eb216a4620cfa55c5cc67bd09dc64ff24`0xb74eE901c2B0A04D75d38f7f4722e8a848E613B9`https://my.coinapp.co/earn~~<?xml version="1.0" encoding="UTF-8"?>
<feed xml:lang="en-US" xmlns="http://www.w3.org/2005/Atom">~~develop```
```

💬 okorye commented on issue "Error unlocking wallet: some keys decrypt but not all. your wallet file may be corrupt. ":
(https://github.com/bitcoin/bitcoin/issues/29789#issuecomment-2052716720)
> ### Is there an existing issue for this?
>
> - [X] I have searched the existing issues
>
> ### Current behaviour
>
> My personal computer crashed and I've already imported "wallet.dat" in the latest Bitcoin Core v26.0.0. It takes time to synchronize the wallet that was about 6 days.
>
> Anyhow, now my wallet is updated and I can see the correct balance of my Bitcoin in the "Overview section". I have all the needed information which I took as a backup such as "passphrases" and "wallet.dat
...

🤔 rkrux reviewed a pull request: "test: Add test case for spending bare multisig"
(https://github.com/bitcoin/bitcoin/pull/29120#pullrequestreview-1999055252)
tACK [9c7b2d0](https://github.com/bitcoin/bitcoin/pull/29120/commits/9c7b2d0d37b31f01403f7a0f2ea25b60b126c841)

Make successful, all functional tests successful.
Asked a question for clarity.

💬 rkrux commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1563742219)
@BrandonOdiwuor For my understanding, why is prepending `bytes(CScript([OP_0]))` to the script sig required after having gone through `sign_input_legacy`?

👍 rkrux approved a pull request: "tests: improve wallet multisig descriptor test and docs"
(https://github.com/bitcoin/bitcoin/pull/29154#pullrequestreview-1999178739)
tACK [8a2021c](https://github.com/bitcoin/bitcoin/pull/29154/commits/8a2021c0f4f40f6f8a37dd5619ecfa3f38084679)

I am in favour of this change because it adds verbosity that makes it easier to go through the code. As mentioned in the PR description, I, too, don't see a harm in including key origin information.

💬 rkrux commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1563833792)
+1 for getting rid of the `/1/*` hardcodings!

💬 rkrux commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1563835474)
`m` could be different in each test run because it uses `random.sample` as I found out while debugging this tst.. For thoroughness, would it be more robust to cover all the orderings instead? In this case it's going to be 2 only because `self.M = 2`.

💬 maflcko commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#discussion_r1563849606)
```suggestion
export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
```

unrelated, but this was fixed a while ago

💬 maflcko commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2053567442)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68020#c1

👍 rkrux approved a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-1999196598)
tACK [5023b47](https://github.com/bitcoin/bitcoin/pull/29156/commits/5023b47e85161a0b35839ce03c7b5d55ff1cd4c1)

Build and all functional tests successful. I like the clarity and the expressiveness with which this test has been written, and believe this is a good addition. Thanks for coming up with this @mjdietzx!

Suggested adding a log message in the last portion of the test.

💬 rkrux commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1563887374)
Most of the code before this one is very similar to `wallet_multisig_descriptor_psbt.py` test - `participants_create_multisigs` is different though.
As pointed out in other reviews as well, I feel this test can go in the same file and some commonalities can be extracted, but I don't think this is blocking though.

💬 rkrux commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1563885340)
```
2024-04-13T08:31:05.463000Z TestFramework (INFO): At block height >= 128 this multisig is 3-of-4
2024-04-13T08:31:05.473000Z TestFramework (INFO): Check that the time-locked transaction is too immature to spend...
2024-04-13T08:31:05.614000Z TestFramework (INFO): Check that balances are correct after the transaction has been included in a block.
```

The first time I ran this test, I felt there should be another log message in between that displays something like "Generating ABC blocks
...

💬 rkrux commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1563886096)
> then wallet would not process that psbt as complete with 3 valid signatures like I expected

Do you mean the 4-signature transaction was rejected?

✅ maflcko closed a pull request: "Update unix build instructions."
(https://github.com/bitcoin/bitcoin/pull/29864)

💬 maflcko commented on pull request "Update unix build instructions.":
(https://github.com/bitcoin/bitcoin/pull/29864#issuecomment-2053591846)
You will have to check out the documentation of the commit you are trying to compile.

For example, if you want to compile 27.x, you will have to use https://github.com/bitcoin/bitcoin/blob/27.x/doc/build-unix.md , not the one of master.

💬 maflcko commented on pull request "Update unix build instructions.":
(https://github.com/bitcoin/bitcoin/pull/29864#discussion_r1563908042)
This hasn't been needed for a long time. You are likely using the wrong docs to compile the wrong commit?

📝 theStack opened a pull request: "util: remove unused cpp-subprocess options"
(https://github.com/bitcoin/bitcoin/pull/29865)
The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class:
https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020
Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to b
...