💬 maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2479737751)
The failure happens intermittently. How often did you try?

💬 maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2479740952)
It should happen on 10% of the runs. Not sure if you have to rebase.

Let me know if it doesn't reproduce.

🤔 murchandamus reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2421497661)
This section of the code is fairly arcane and hard to read. It would help me (and probably other reviewers) if there were more commentary on why things are being done and what is actually happening, especially because a lot of the involved variables are never explicitly denominated.

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1832978940)
What does "sols" stand for beside solar days on Mars? Is this intended to refer to "solvers"? If so, abbreviating that just seems unnecessarily confusing.

```suggestion
std::vector<std::vector<unsigned char>> solvers;
```

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841132532)
```suggestion
// These are only spendable if the witness script is also spendable as a scriptPubKey
```

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841207696)
Could we maybe give a few pointers what’s going on here?
```suggestion
if (GetPubKey(key_id, pubkey) && pubkey.IsCompressed() && HaveKey(key_id)) {
// Insert the output script for a P2WPKH output
spks.insert(script);
// Insert the output script for a P2SH-P2WPKH output
spks.insert(GetScriptForDestination(ScriptHash(script)));
```

Again, mostly guessing. :shrug:

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844326937)
Did you mean?

```suggestion
// We need to have the witness script to consider the P2WSH output script to be spendable.
```

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844300642)
I was first confused by the snake_case apparently being used as a function below. A comment would have helped me figure this out a bit faster. Maybe something in the vein of:

```suggestion
// Assigns an unnamed lambda function which checks that all keys are compressed
const auto& func_all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
return std::all_of(keys.cbegin(), keys.cend(),
[](const auto& key) { return key.size() == 33; });
};

...

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844302843)
Could this comment please be more specific? What sorts of scripts are being stored in this map? Input scripts, output scripts, redeemscripts, witness programs, witness scripts, all of the above, a subset of the above, etc.?

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844306218)
This comment is confusing, because once `spks` refers to the variable, and once "spks" seems to refer to output scripts. If I understand this right:

```suggestion
// We will check these later after `spks` has been updated with scriptPubKeys from the processed scripts.
```

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841210613)
Are we talking about private keys here? Or public keys? E.g. clearly we would need all three public keys, but only two private keys to spend a 2-of-3 bare multisig.

This is talking about bare multisig, right? Would something like this make sense?

```suggestion
// Bare multisig outputs are only spendable if we have [public|private|public and private] keys that appear in the output script
```

💬 murchandamus commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844338459)
What is our expectation to be happening when `uint160(sols[0])` is called here? Could we maybe pull apart these three conversations in a conditional statement into two lines where the first line provides some insight on what we are converting here?

Would I be right in guessing that a `CKeyID` is the hash of a public key?

🤔 furszy reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2439445784)
There is a bug in the second loop. Before adding the P2WSH program to the spks set, it is necessary to verify that the underlying witness script does not contain any uncompressed keys, as these are prohibited under segwit rules.
Crafted a test exercising this behavior https://github.com/furszy/bitcoin-core/commit/3a0d127343320cc00f13cef96fad7f3b1bd3335a.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2479769510)
@maflcko Interestingly I got the error after 5 runs.
<details>

<summary>logs</summary>

```terminal
2024-11-15T19:20:59.167000Z TestFramework (INFO): Test issue 22670 ApproximateBestSubset bug
2024-11-15T19:21:00.082000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/abubakarismail/Desktop/Work/
...

👍 TheCharlatan approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2439476734)
Re-ack 9aa50152c1cfa1c41215b2b51ed7a516aa67137a

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479815069)
> that will be trivially true unless current_tip and tip_hash are both 0

That's how I use it in https://github.com/Sjors/bitcoin/pull/49 when the Template Provider spins up its main thread:

```cpp
void Sv2TemplateProvider::ThreadSv2Handler()
{
// Wait for the node chainstate to be ready if needed.
auto tip{m_mining.waitTipChanged(uint256::ZERO)};
Assert(tip.hash != uint256::ZERO);
```

> But It would seem cleaner to just call `blockTip()` when the tip is first loaded so
...

💬 dergoegge commented on issue "RFC: Adopt C++ Safe Buffers?":
(https://github.com/bitcoin/bitcoin/issues/31272#issuecomment-2479815730)
> My understanding is that only libc++ offers such a hardened build right now

`-D_GLIBCXX_ASSERTIONS` appears to enable the same (or similar) for libstdc++, see [here](https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html#precondition-checks-for-c-standard-library-calls).

✅ furszy closed a pull request: "test: group executed tests within the same directory"
(https://github.com/bitcoin/bitcoin/pull/31291)

💬 furszy commented on pull request "test: group executed tests within the same directory":
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2479873882)
Hmm, okay. Since the inclusion of CTest, we can run unit tests in parallel, but they are executed in standalone processes, which makes this approach impractical. Maybe we could address this at the CTest level, but I don’t have much time to spend on it.
If someone wants to tackle it, all yours. Up for grabs.

👍 tdb3 approved a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2439672105)
code review re ACK 111465d72dd35e42361fc2a089036f652417ed37