💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309)
I wonder if it is clearer to make `m_tip_block` nullopt, instead of a magic value? Would be nice to see the diff and pick the better one.
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309)
I wonder if it is clearer to make `m_tip_block` nullopt, instead of a magic value? Would be nice to see the diff and pick the better one.
🤔 furszy reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2434167216)
Half way through it. Small comments so far.
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2434167216)
Half way through it. Small comments so far.
💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841007320)
nit:
if you move this to use an structured binding, could use the script id directly instead of calculating it on every ´ScriptHash(script)´ call.
e.g.
```c++
for (const auto& [id, script] : mapScripts) {
// ... stuff ...
spks.insert(GetScriptForDestination(ScriptHash(id)));
// ^^ this does not perform the extra Hash160(script) call.
}
```
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1841007320)
nit:
if you move this to use an structured binding, could use the script id directly instead of calculating it on every ´ScriptHash(script)´ call.
e.g.
```c++
for (const auto& [id, script] : mapScripts) {
// ... stuff ...
spks.insert(GetScriptForDestination(ScriptHash(id)));
// ^^ this does not perform the extra Hash160(script) call.
}
```
💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844166304)
Note for reviewers:
This is because we store scripts (witness programs, witness scripts and redeem scripts) in a map whose key is a `CScriptID` -> which is a Hash160 of the script -> which is a `RIPEMD160(SHA256(script))`.
As P2WSH are in the form of `OP_0 <SHA256(witness_script)>`, the SHA256 part is already there and we only need to perform the RIPEMD160 calculation to obtain the `CScriptID`.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844166304)
Note for reviewers:
This is because we store scripts (witness programs, witness scripts and redeem scripts) in a map whose key is a `CScriptID` -> which is a Hash160 of the script -> which is a `RIPEMD160(SHA256(script))`.
As P2WSH are in the form of `OP_0 <SHA256(witness_script)>`, the SHA256 part is already there and we only need to perform the RIPEMD160 calculation to obtain the `CScriptID`.
💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844244829)
Is this being tested? It seems we have P2WSH(multisig) tests inside `ismine_tests.cpp` and `wallet_migration.py` but these type of script should be covered inside the first loop already. Because if the wallet owns the P2WSH, it will also contain the inner multisig script, which will appear in the first loop.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844244829)
Is this being tested? It seems we have P2WSH(multisig) tests inside `ismine_tests.cpp` and `wallet_migration.py` but these type of script should be covered inside the first loop already. Because if the wallet owns the P2WSH, it will also contain the inner multisig script, which will appear in the first loop.
💬 adamandrews1 commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2479591513)
I am working on this issue, will issue a patch soon.
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2479591513)
I am working on this issue, will issue a patch soon.
💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1844247558)
It is useful to because running `build/test/functional/test_runner.py --help`, will describe this new feature for users. If this is removed, it will not be documented, and future users may not learn about it.
Therefore, I will be leaving it as is.
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1844247558)
It is useful to because running `build/test/functional/test_runner.py --help`, will describe this new feature for users. If this is removed, it will not be documented, and future users may not learn about it.
Therefore, I will be leaving it as is.
💬 gmart7t2 commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2479636045)
Here's the use case:
I have a descriptor that I know has never been used. I want to import it. I don't need any rescan, because I know it hasn't been used.
In this case I don't care about variations in block times because 2 hours before "never used" is still "never used".
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2479636045)
Here's the use case:
I have a descriptor that I know has never been used. I want to import it. I don't need any rescan, because I know it hasn't been used.
In this case I don't care about variations in block times because 2 hours before "never used" is still "never used".
💬 furszy commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844279607)
How can we end up with a P2SH or a witness program inside the witness script?
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844279607)
How can we end up with a P2SH or a witness program inside the witness script?
💬 Sjors commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1844280694)
Added the space.
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1844280694)
Added the space.
💬 sipa commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2479659321)
I don't think it's unreasonable to have a way to signal "never used".
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2479659321)
I don't think it's unreasonable to have a way to signal "never used".
💬 ryanofsky commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479683128)
> It's useful to be able to wait for an active chaintip to appear, by calling waitTipChanged(uint256::ZERO);.
I think the bug here is just that blockTip() is not called on startup so m_tip_block is not set when node starts up, only after it starts up and then a new block get connected. Otherwise the waitTipChanged method is already written to provide desired behavior by waiting until there is any new tip and the new tip is not zero:
```c++
notifications().m_tip_block_cv.wait_
...
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479683128)
> It's useful to be able to wait for an active chaintip to appear, by calling waitTipChanged(uint256::ZERO);.
I think the bug here is just that blockTip() is not called on startup so m_tip_block is not set when node starts up, only after it starts up and then a new block get connected. Otherwise the waitTipChanged method is already written to provide desired behavior by waiting until there is any new tip and the new tip is not zero:
```c++
notifications().m_tip_block_cv.wait_
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2479709034)
@DrahtBot I think C.I failure here is unrelated?
I was not successful in my attempt to recreate it locally.
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2479709034)
@DrahtBot I think C.I failure here is unrelated?
I was not successful in my attempt to recreate it locally.
💬 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?
(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.
(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.
(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;
```
(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
```
(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:
(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.
```