👍 ryanofsky approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2439241478)
Code review ACK b28972cd85e4472b386349d6cda8c233faeffd4f
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2439241478)
Code review ACK b28972cd85e4472b386349d6cda8c233faeffd4f
💬 ryanofsky commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1844221466)
In commit "Add destroy to BlockTemplate schema" (b28972cd85e4472b386349d6cda8c233faeffd4f)
note: Missing a space before the number this line
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1844221466)
In commit "Add destroy to BlockTemplate schema" (b28972cd85e4472b386349d6cda8c233faeffd4f)
note: Missing a space before the number this line
📝 Sjors opened a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)
It's useful to be able to wait for an active chaintip to appear, by calling ` waitTipChanged(uint256::ZERO);`.
Unfortunately this doesn't work out of the box, because `notifications().m_tip_block` is `ZERO` until a new block arrives.
Additionally we need an early return before calling `wait_for`.
(https://github.com/bitcoin/bitcoin/pull/31297)
It's useful to be able to wait for an active chaintip to appear, by calling ` waitTipChanged(uint256::ZERO);`.
Unfortunately this doesn't work out of the box, because `notifications().m_tip_block` is `ZERO` until a new block arrives.
Additionally we need an early return before calling `wait_for`.
💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479536761)
cc @TheCharlatan life would be siugnifancly better if `notifications().m_tip_block` just jumped to the tip as soon as there is one, rather than wait for the next block to come in. That applies to #31283 too.
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2479536761)
cc @TheCharlatan life would be siugnifancly better if `notifications().m_tip_block` just jumped to the tip as soon as there is one, rather than wait for the next block to come in. That applies to #31283 too.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2479552572)
Added early return logic. Similar to #31297 this would be less tedious if `notifications().m_tip_block` was set the tip earlier.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2479552572)
Added early return logic. Similar to #31297 this would be less tedious if `notifications().m_tip_block` was set the tip earlier.
💬 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.