💬 fanquake commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660241633)
@MarcoFalke does this fix #28057 in it's current state?
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660241633)
@MarcoFalke does this fix #28057 in it's current state?
💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280577467)
In 87abceea:
Maybe better `WALLET_FLAG_GLOBAL_HD_KEY`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280577467)
In 87abceea:
Maybe better `WALLET_FLAG_GLOBAL_HD_KEY`?
💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280568616)
Or.. another possible patch to not mix the keys reading code with the upgrade code could be to read the xpub root key alone at the end of the spkm loading process:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
--- a/src/wallet/walletdb.cpp (revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
+++ b/src/wallet/walletdb.cpp (date 1690893250659)
@@ -966,6 +966,31 @@
result = std::max(result, ckey_res.m_result);
num_ckeys = ckey_res.m_records;
+
...
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280568616)
Or.. another possible patch to not mix the keys reading code with the upgrade code could be to read the xpub root key alone at the end of the spkm loading process:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
--- a/src/wallet/walletdb.cpp (revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
+++ b/src/wallet/walletdb.cpp (date 1690893250659)
@@ -966,6 +966,31 @@
result = std::max(result, ckey_res.m_result);
num_ckeys = ckey_res.m_records;
+
...
💬 sipa commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660269660)
I think the only correct approach to implement something like this, is to keep the unbroadcast transaction within the wallet (in a special unbroadcast state until it's echoed back). In this unbroadcast state, rebroadcasts can happen (directly to sensitive relay peers only, bypassing the mempool), but outputs are not available for creating descendant transactions.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660269660)
I think the only correct approach to implement something like this, is to keep the unbroadcast transaction within the wallet (in a special unbroadcast state until it's echoed back). In this unbroadcast state, rebroadcasts can happen (directly to sensitive relay peers only, bypassing the mempool), but outputs are not available for creating descendant transactions.
💬 ryanofsky commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1660271019)
> If we end up with an opinion from the BLDF then maybe we can consider making an addition to our license, if necessary.
+1. If we have professional advise to change the license or add a separate policy document or agreement like a CLA, we should consider doing that. But we shouldn't freelance and add legal speculation to developer documentation.
In this case and in general I think a good strategy is to:
- First, focus on doing the right thing morally. If contribution includes content t
...
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1660271019)
> If we end up with an opinion from the BLDF then maybe we can consider making an addition to our license, if necessary.
+1. If we have professional advise to change the license or add a separate policy document or agreement like a CLA, we should consider doing that. But we shouldn't freelance and add legal speculation to developer documentation.
In this case and in general I think a good strategy is to:
- First, focus on doing the right thing morally. If contribution includes content t
...
💬 MarcoFalke commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660271611)
It did fix the crash when I left the Concept ACK https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1545962463
Happy to re-test after the next ACK or after merge.
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660271611)
It did fix the crash when I left the Concept ACK https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1545962463
Happy to re-test after the next ACK or after merge.
💬 glozow commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1660272298)
@darosior thanks for the data! The proportion of transactions with descendants seems pretty high, so my comment about ignoring stuff with descendants is probably not a good idea.
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1660272298)
@darosior thanks for the data! The proportion of transactions with descendants seems pretty high, so my comment about ignoring stuff with descendants is probably not a good idea.
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660280410)
> but outputs are not available for creating descendant transactions.
In theory they could be used to create transactions, under the assumption that created transactions generally and eventually relay, but that can be fixed independent of the relay issue here. Pretty sure there is a tracking issue about this already, which likely affects `-nowalletbroadcast` and `-spendzeroconfchange` users.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660280410)
> but outputs are not available for creating descendant transactions.
In theory they could be used to create transactions, under the assumption that created transactions generally and eventually relay, but that can be fixed independent of the relay issue here. Pretty sure there is a tracking issue about this already, which likely affects `-nowalletbroadcast` and `-spendzeroconfchange` users.
🤔 theStack reviewed a pull request: "blockstorage: Drop legacy -txindex check"
(https://github.com/bitcoin/bitcoin/pull/28195#pullrequestreview-1556965654)
Concept ACK
There are still two instances where blocks_path can be used (sometimes it's quite annoying that python also allows single quote strings):
```
test/functional/feature_pruning.py: self.prunedir = os.path.join(self.nodes[2].chain_path, 'blocks', '')
test/functional/wallet_backup.py: shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'blocks'))
```
With my very limited sed skills, I'd just add another line
`sed -i 's|].chain_path, 'blocks'|].blocks_path|g' $(
...
(https://github.com/bitcoin/bitcoin/pull/28195#pullrequestreview-1556965654)
Concept ACK
There are still two instances where blocks_path can be used (sometimes it's quite annoying that python also allows single quote strings):
```
test/functional/feature_pruning.py: self.prunedir = os.path.join(self.nodes[2].chain_path, 'blocks', '')
test/functional/wallet_backup.py: shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'blocks'))
```
With my very limited sed skills, I'd just add another line
`sed -i 's|].chain_path, 'blocks'|].blocks_path|g' $(
...
💬 MarcoFalke commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280656421)
> The sequence number isn't incremented until TransactionAddedToMempool() fires later.
Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
Basically, I am wondering what happens if you reorder `t=2` and `t=3` in your example.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280656421)
> The sequence number isn't incremented until TransactionAddedToMempool() fires later.
Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
Basically, I am wondering what happens if you reorder `t=2` and `t=3` in your example.
💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-1660344850)
Rebased!
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-1660344850)
Rebased!
💬 dergoegge commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1660350895)
I think I'd prefer continuing to use the defaults but I also don't use the runner much for generating, so no super strong opinion. I would assume that the defaults are based on data/heuristics and probably work well most of the time, so maybe make this optional?
> Sometimes a libFuzzer setting like -use_value_profile=1 helps [0], sometimes it hurts [1].
> By picking a random value, it is ensured that at least some of the runs will have the beneficial configuration set.
Wouldn't this only
...
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1660350895)
I think I'd prefer continuing to use the defaults but I also don't use the runner much for generating, so no super strong opinion. I would assume that the defaults are based on data/heuristics and probably work well most of the time, so maybe make this optional?
> Sometimes a libFuzzer setting like -use_value_profile=1 helps [0], sometimes it hurts [1].
> By picking a random value, it is ensured that at least some of the runs will have the beneficial configuration set.
Wouldn't this only
...
💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1660386703)
Except for `mutate_depth`, 70% of the time the default value is used for each setting. Happy to change `mutate_depth` as well, which would mean in `.7**3` runs you get a "vanilla" libfuzzer.
I'd say the main goal here is to prevent a case where a bug isn't found at all (or only after a "outlier" long time) due to the default settings. I don't think we've seen such a bug in reality, so I am happy to close this pull.
Regardless, `max_len` should probably be considered separate, where the goa
...
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1660386703)
Except for `mutate_depth`, 70% of the time the default value is used for each setting. Happy to change `mutate_depth` as well, which would mean in `.7**3` runs you get a "vanilla" libfuzzer.
I'd say the main goal here is to prevent a case where a bug isn't found at all (or only after a "outlier" long time) due to the default settings. I don't think we've seen such a bug in reality, so I am happy to close this pull.
Regardless, `max_len` should probably be considered separate, where the goa
...
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1660402481)
@jamesob Mind doing a re-ACK (trivial). Otherwise I wonder if anything is left to be done here? Is this waiting on someone's NACK or ACK?
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1660402481)
@jamesob Mind doing a re-ACK (trivial). Otherwise I wonder if anything is left to be done here? Is this waiting on someone's NACK or ACK?
💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1660405202)
Rebased on the latest #https://github.com/bitcoin/bitcoin/pull/28060
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1660405202)
Rebased on the latest #https://github.com/bitcoin/bitcoin/pull/28060
💬 Sjors commented on pull request "rpc, wallet: addhdseed, infer seed when importing descriptor with xpub ":
(https://github.com/bitcoin/bitcoin/pull/23544#issuecomment-1660421353)
This is very out of date and it's not on my priority list at the moment. Happy to review if someone else takes it over.
(https://github.com/bitcoin/bitcoin/pull/23544#issuecomment-1660421353)
This is very out of date and it's not on my priority list at the moment. Happy to review if someone else takes it over.
✅ Sjors closed a pull request: "rpc, wallet: addhdseed, infer seed when importing descriptor with xpub "
(https://github.com/bitcoin/bitcoin/pull/23544)
(https://github.com/bitcoin/bitcoin/pull/23544)
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280715228)
> Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
The mempool value is incremented when TrAddedTMP is queued, not when it's eventually run -- the increment happens first, then the result of the increment is passed through and queued up. So I don't think there's any racy here.
> Perhaps it makes sense to use `m_pool.GetSequence() + 1` here?
I think it would make more sense to change `info_for_relay`?
```
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280715228)
> Doesn't that mean there are races? (There is no guarantee that `TransactionAddedToMempool()` will always run in the near future)?
The mempool value is incremented when TrAddedTMP is queued, not when it's eventually run -- the increment happens first, then the result of the increment is passed through and queued up. So I don't think there's any racy here.
> Perhaps it makes sense to use `m_pool.GetSequence() + 1` here?
I think it would make more sense to change `info_for_relay`?
```
...
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280721691)
I switched to using util::Result in my last rebase a few months ago.
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280721691)
I switched to using util::Result in my last rebase a few months ago.
💬 MarcoFalke commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280757943)
Pretty sure this is wrong, as you are not checking `!res.value()`. (Previously it threw, now it silently continues. Now it only throws when no `ExternalSignerScriptPubKeyMan` is available.)
This can be fixed non-confusingly by using `Result<void>`
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280757943)
Pretty sure this is wrong, as you are not checking `!res.value()`. (Previously it threw, now it silently continues. Now it only throws when no `ExternalSignerScriptPubKeyMan` is available.)
This can be fixed non-confusingly by using `Result<void>`