🤔 furszy reviewed a pull request: "test: check backup from `migratewallet` can be successfully restored"
(https://github.com/bitcoin/bitcoin/pull/28257#pullrequestreview-1573714741)
While I'm a concept ACK, I think that the approach isn't the best.
The code is walking through the node's directory, restoring all the backup files without checking if the restored wallet is correct or not.
The main point behind a backup test should be to assert that the restored wallet has the same balance and it watches the same scripts as before. Not only check that the backup file can be opened (the wallet could be empty or missing some information).
Also, test cases should be indep
...
(https://github.com/bitcoin/bitcoin/pull/28257#pullrequestreview-1573714741)
While I'm a concept ACK, I think that the approach isn't the best.
The code is walking through the node's directory, restoring all the backup files without checking if the restored wallet is correct or not.
The main point behind a backup test should be to assert that the restored wallet has the same balance and it watches the same scripts as before. Not only check that the backup file can be opened (the wallet could be empty or missing some information).
Also, test cases should be indep
...
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291449745)
That seems a bit out of place in this PR.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291449745)
That seems a bit out of place in this PR.
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291453343)
That’s right, I’ll remove the unnecessary block generation
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291453343)
That’s right, I’ll remove the unnecessary block generation
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291466087)
Agree, updated the logging
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291466087)
Agree, updated the logging
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291467564)
Good point, added a `bumpmocktime` to test framework, I'll probably use it in other tests in the future.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291467564)
Good point, added a `bumpmocktime` to test framework, I'll probably use it in other tests in the future.
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291469104)
Sorree! Added explanation in the comment.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291469104)
Sorree! Added explanation in the comment.
💬 glozow commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1291488191)
No, as it means skipping `LimitMempoolSize` within `Finalize()` and we're not always guaranteed to call `SubmitPackage` afterwards (e.g. if we quit early).
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1291488191)
No, as it means skipping `LimitMempoolSize` within `Finalize()` and we're not always guaranteed to call `SubmitPackage` afterwards (e.g. if we quit early).
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291490820)
Every once in a while we’ll get a signature that’s gonna be one byte smaller, so a little bit of a buffer will prevent intermittent failures. I reduced the buffers to 1%, though.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291490820)
Every once in a while we’ll get a signature that’s gonna be one byte smaller, so a little bit of a buffer will prevent intermittent failures. I reduced the buffers to 1%, though.
💬 dergoegge commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291514800)
Wouldn't it make more sense to have `bumpmocktime` on TestNode and set `self.mocktime` when ever `setmocktime` is called?
* allows for node independent mocktime bumping
* allows to use an initial mocktime other than time.time()
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291514800)
Wouldn't it make more sense to have `bumpmocktime` on TestNode and set `self.mocktime` when ever `setmocktime` is called?
* allows for node independent mocktime bumping
* allows to use an initial mocktime other than time.time()
💬 furszy commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674988814)
> On master branch [b2ec032](https://github.com/bitcoin/bitcoin/commit/b2ec0326fd76e64a6d0d7e4745506b29f60d0be5) I'm still getting some mixed signals from this command:
>
> in the logs:
>
> ```
> 2023-08-11T13:43:34Z ERROR: FillableSigningProvider::AddCScript(): redeemScripts > 520 bytes are invalid
> ```
>
> json-rpc response:
>
> ```
> {
> "address": "3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc",
> "redeemScript": "60... ...ae",
> "descriptor": "addr(3JDRACdkEs41yxtYLi6kiqQJdQHL
...
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674988814)
> On master branch [b2ec032](https://github.com/bitcoin/bitcoin/commit/b2ec0326fd76e64a6d0d7e4745506b29f60d0be5) I'm still getting some mixed signals from this command:
>
> in the logs:
>
> ```
> 2023-08-11T13:43:34Z ERROR: FillableSigningProvider::AddCScript(): redeemScripts > 520 bytes are invalid
> ```
>
> json-rpc response:
>
> ```
> {
> "address": "3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc",
> "redeemScript": "60... ...ae",
> "descriptor": "addr(3JDRACdkEs41yxtYLi6kiqQJdQHL
...
💬 stevenroose commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1675000173)
@furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too.. I'll do that for now. But anyway, someone getting hold of your hard drive shouldn't learn your wallet metadata, I think that's a good principle.
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1675000173)
@furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too.. I'll do that for now. But anyway, someone getting hold of your hard drive shouldn't learn your wallet metadata, I think that's a good principle.
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291528300)
Thanks, good idea. I went with:
```python
def assert_spends_only_parents(self, tx, parent_txids):
parent_checklist = parent_txids.copy()
number_inputs = len(tx["decoded"]["vin"])
assert_equal(number_inputs, len(parent_txids))
for i in range(number_inputs):
txid_of_input = tx["decoded"]["vin"][i]["txid"]
assert txid_of_input in parent_checklist
parent_checklist.remove(txid_of_input)
```
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291528300)
Thanks, good idea. I went with:
```python
def assert_spends_only_parents(self, tx, parent_txids):
parent_checklist = parent_txids.copy()
number_inputs = len(tx["decoded"]["vin"])
assert_equal(number_inputs, len(parent_txids))
for i in range(number_inputs):
txid_of_input = tx["decoded"]["vin"][i]["txid"]
assert txid_of_input in parent_checklist
parent_checklist.remove(txid_of_input)
```
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291532701)
The grandparent transaction has a higher feerate, so we only need to bump the parent. We check that if we get the complete chain of unconfirmed transactions, that the resulting feerate is above our target.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291532701)
The grandparent transaction has a higher feerate, so we only need to bump the parent. We check that if we get the complete chain of unconfirmed transactions, that the resulting feerate is above our target.
💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1675009460)
Re https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674826021
> Presumably when de-globalizing, these functions will need the kernel anyway.
I am not sure how we could go about registering our signal handlers without the shutdown global.
> It would document/force the correct creation order
I originally wanted to to introduce this in a separate PR (fixing the order for gui initialization), but I think it makes more sense here, yes.
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1675009460)
Re https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674826021
> Presumably when de-globalizing, these functions will need the kernel anyway.
I am not sure how we could go about registering our signal handlers without the shutdown global.
> It would document/force the correct creation order
I originally wanted to to introduce this in a separate PR (fixing the order for gui initialization), but I think it makes more sense here, yes.
📝 TheCharlatan converted_to_draft a pull request: "kernel: Run sanity checks on context construction"
(https://github.com/bitcoin/bitcoin/pull/28228)
Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r882182549) by @theuni in the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) first introducing the `kernel::Context`.
Since this touches the daemon initialization logic, also fix a bug where it would be possible for a terminat
...
(https://github.com/bitcoin/bitcoin/pull/28228)
Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r882182549) by @theuni in the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) first introducing the `kernel::Context`.
Since this touches the daemon initialization logic, also fix a bug where it would be possible for a terminat
...
💬 theuni commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1675023954)
We've debated the CNode* passing/abstracting thing ever since CConnman was merged. This has come up over and over so it's definitely not new. And the big problem is: everyone has their own take. The question of "should we use CNode or CConnman as the interface for network stuff" could be debated forever. I think the only correct answer is "not both arbitrarily", which is what we do now.
I commend @dergoegge for working on this. It's tough and requires picking an approach and sticking to it.
...
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1675023954)
We've debated the CNode* passing/abstracting thing ever since CConnman was merged. This has come up over and over so it's definitely not new. And the big problem is: everyone has their own take. The question of "should we use CNode or CConnman as the interface for network stuff" could be debated forever. I think the only correct answer is "not both arbitrarily", which is what we do now.
I commend @dergoegge for working on this. It's tough and requires picking an approach and sticking to it.
...
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291548464)
I also updated the `miniminer_1p1c` test to match the transaction numbers to the indices
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291548464)
I also updated the `miniminer_1p1c` test to match the transaction numbers to the indices
💬 darosior commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1291552809)
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1291552809)
Thanks, done.
💬 darosior commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#issuecomment-1675038081)
> I'd prefer if you summarize what the PR does in the description.
Fair enough, my PR description was sloppy. Done.
> Mostly the fact that these assertions are only correct if we use the default CCoinsView
I would disagree here. While it's true the code is "correct" as in the fuzz target doesn't crash, the logic itself is not correct:
- It's asserting something that is always false, namely that the cache is always in sync with the backend.. Which is antithetical to using a cache in the
...
(https://github.com/bitcoin/bitcoin/pull/28215#issuecomment-1675038081)
> I'd prefer if you summarize what the PR does in the description.
Fair enough, my PR description was sloppy. Done.
> Mostly the fact that these assertions are only correct if we use the default CCoinsView
I would disagree here. While it's true the code is "correct" as in the fuzz target doesn't crash, the logic itself is not correct:
- It's asserting something that is always false, namely that the cache is always in sync with the backend.. Which is antithetical to using a cache in the
...
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291565216)
That’s a good idea, but I’m gonna leave this, because this matches how we interact with the mempool entries.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291565216)
That’s a good idea, but I’m gonna leave this, because this matches how we interact with the mempool entries.