💬 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

👋 ryanofsky's pull request is ready for review: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260)

📝 adamandrews1 opened a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298)
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.

fixes #25980

💬 mzumsande commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1844527950)
done (in a slightly adjusted form)

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#issuecomment-2479964213)
Pushed a lot of fixes and updates over the past week and marking this PR as no longer wip/draft. This PR is just a refactoring and doesn't change runtime behavior, but it should be a good start to having better defined settings with clear types and default values, avoiding confusion and bugs caused by the current settings API seen in #30529 and #31212 and other PRs, and making API more extensible to support custom types and validation in the future.

💬 mzumsande commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2479974458)
Updated Release note and added a commit that adds a functional test for various aspects of `-port` that have been discussed in this PR an the issue.

When it comes to the question whether the issue should be fixed in 28.1, I guess it also comes down to how widespread we think a scenario with `-port` and a manual `torrc` is, vs how many more people doing local testing setups would be affected by the `-port` change from 28.0.
I can't say I have a good feeling for either - to be honest I would h
...

⚠️ techy2 opened an issue: "depends build fails - libevent not in CMakeLists"
(https://github.com/bitcoin/bitcoin/issues/31299)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

CMake Error: The source directory "/master/gitrepo/bitcoin/depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.12-stable-9d8e3ddea5c/build" does not appear to contain CMakeLists.txt.


### Expected behaviour

expect it to build

### Steps to reproduce

git clone https://github.com/bitcoin/bitcoin
cd bitcoin
git checkout v28.0
cd depends
make


### Relevant log output

/master/gitrepo
...

💬 techy2 commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2479997928)
that's 128 G ram

📝 adamandrews1 converted_to_draft a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298)
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.

fixes #25980

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844630125)
Using `importscript`, you can import arbitrary scripts that can appear as output scripts. It is thus possible to import a P2WSH script which contains the hash of a P2SH script or a witness program. The user can then use `importscripts` again to import those particular scripts.

Most of the edge cases revolve around the user doing something insane, but because they were allowed, we have to handle them.

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844642514)
**sol**ution**s**

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844644393)
Generally "keys" refers to private keys and public keys are typically specified as such or as "pubkey".

In this instance, multisigs are only spendable if we have all of their private keys, regardless of the context the multisig appears in.