💬 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:
(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.
```
(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; });
};
...
(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.?
(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.
```
(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
```
(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?
(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.
(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/
...
(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
(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
...
(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).
(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)
(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.
(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
(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)
(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
(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)
(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.
(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
...
(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
...