💬 dergoegge commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674649044)
> then avoiding (or at least minimising) having to go through shared data structures will be necessary
I could agree on s/necessary/more performant, but with the shared data structure design it would really only cause contention for the look ups which is really not that bad. i.e.
```c++
// in CConnman::DoX(NodeId)
{
LOCK(m_nodes_mutex);
// do lookup
}
// do work on the CNode
```
> Keeping our data structures in sync is already challenging,
It's challenging rig
...
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674649044)
> then avoiding (or at least minimising) having to go through shared data structures will be necessary
I could agree on s/necessary/more performant, but with the shared data structure design it would really only cause contention for the look ups which is really not that bad. i.e.
```c++
// in CConnman::DoX(NodeId)
{
LOCK(m_nodes_mutex);
// do lookup
}
// do work on the CNode
```
> Keeping our data structures in sync is already challenging,
It's challenging rig
...
📝 brunoerg opened a pull request: "test: check backup from `migratewallet` can be successfully restored"
(https://github.com/bitcoin/bitcoin/pull/28257)
`migratewallet` migrates the wallet to a descriptor one. During the process, it generates a backup file of the wallet in case of an incorrect migration. This PR adds test to check if the backup file can be successfully restored.
(https://github.com/bitcoin/bitcoin/pull/28257)
`migratewallet` migrates the wallet to a descriptor one. During the process, it generates a backup file of the wallet in case of an incorrect migration. This PR adds test to check if the backup file can be successfully restored.
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674741387)
On Fri, Aug 11, 2023 at 02:56:55AM -0700, John Carvalho wrote:
> NACK.
>
> The preceding mempoolfullrbf debate resulted in a Bitcoin Core where mempool policy is a minority voting system.
>
> While I acknowledge that mempool policy is local and any consistency we have across policies is a phenomenon of incentives, AND that the likely proper solution is to REMOVE mempool policy steering from both the Core community and software, here we are nonetheless.
>
> The default config for Core is a da
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674741387)
On Fri, Aug 11, 2023 at 02:56:55AM -0700, John Carvalho wrote:
> NACK.
>
> The preceding mempoolfullrbf debate resulted in a Bitcoin Core where mempool policy is a minority voting system.
>
> While I acknowledge that mempool policy is local and any consistency we have across policies is a phenomenon of incentives, AND that the likely proper solution is to REMOVE mempool policy steering from both the Core community and software, here we are nonetheless.
>
> The default config for Core is a da
...
📝 theuni opened a pull request: "bitcoin-tidy: fix macOS build"
(https://github.com/bitcoin/bitcoin/pull/28258)
[LLVM uses these options](https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L178) for building as well, so there's precedent.
Also fix the shared library extension which was incorrectly being set to dylib.
Thanks to jonatack for reporting and debugging.
(https://github.com/bitcoin/bitcoin/pull/28258)
[LLVM uses these options](https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L178) for building as well, so there's precedent.
Also fix the shared library extension which was incorrectly being set to dylib.
Thanks to jonatack for reporting and debugging.
👍 dergoegge approved a pull request: "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1573687488)
Code review ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9
Nice interface cleanup!
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1573687488)
Code review ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9
Nice interface cleanup!
💬 dergoegge commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1291351347)
(somewhat unrelated to this pull)
Are there any plans to remove the chainman reference from `Chainstate`? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of `Chainstate` and `ChainstateManager` are not well defined.
(It's probably also more or less just an artifact of the de-globalization work)
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1291351347)
(somewhat unrelated to this pull)
Are there any plans to remove the chainman reference from `Chainstate`? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of `Chainstate` and `ChainstateManager` are not well defined.
(It's probably also more or less just an artifact of the de-globalization work)
💬 theuni commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674826021)
Assuming I'm understanding the init order bug, please correct me if I'm not...
If `AppInitBasicSetup()` and `AppInitParameterInteraction()` are relying on globals created by the kernel, perhaps it makes sense for them to require a kernel to be passed in, even if it's unused inside those functions for now?
- It would document/force the correct creation order
- Presumably when de-globalizing, these functions will need the kernel anyway.
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674826021)
Assuming I'm understanding the init order bug, please correct me if I'm not...
If `AppInitBasicSetup()` and `AppInitParameterInteraction()` are relying on globals created by the kernel, perhaps it makes sense for them to require a kernel to be passed in, even if it's unused inside those functions for now?
- It would document/force the correct creation order
- Presumably when de-globalizing, these functions will need the kernel anyway.
💬 pinheadmz 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-1674830104)
On master branch 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(3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc)#x7qnlkt9",
"warnings": [
"Unable to make chosen address type, please ensure no uncompressed public keys are
...
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104)
On master branch 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(3JDRACdkEs41yxtYLi6kiqQJdQHLPDxehc)#x7qnlkt9",
"warnings": [
"Unable to make chosen address type, please ensure no uncompressed public keys are
...
🤔 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)
```