💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674459315)
Thank you for the review @furszy
Updated cfe93642b27cbd4770200b9a6e571e1388ba35dc -> 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc ([contextSanityChecks_0](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_0) -> [contextSanityChecks_1](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_0..contextSanityChecks_1))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/2
...
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674459315)
Thank you for the review @furszy
Updated cfe93642b27cbd4770200b9a6e571e1388ba35dc -> 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc ([contextSanityChecks_0](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_0) -> [contextSanityChecks_1](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_0..contextSanityChecks_1))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/2
...
💬 TheCharlatan commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#issuecomment-1674461756)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28253#issuecomment-1674461756)
Concept ACK
💬 fanquake commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1674463539)
> Nit: stale description.
Updated the description.
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1674463539)
> Nit: stale description.
Updated the description.
🚀 fanquake merged a pull request: "doc: use llvm-config for bitcoin-tidy example"
(https://github.com/bitcoin/bitcoin/pull/28245)
(https://github.com/bitcoin/bitcoin/pull/28245)
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291125153)
@ajtowns I raised a similar concern, but it looks like this is safe: https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291125153)
@ajtowns I raised a similar concern, but it looks like this is safe: https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291139542)
> Should we add an rpc to allow querying the orphanage?
Could do that, as there's a bit of black box-ness here. Though I also think that the ideal situation would be to write unit tests if we're interested in the exact contents of the orphanage.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291139542)
> Should we add an rpc to allow querying the orphanage?
Could do that, as there's a bit of black box-ness here. Though I also think that the ideal situation would be to write unit tests if we're interested in the exact contents of the orphanage.
💬 BitcoinErrorLog commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674487742)
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 dangerous space to design, and we should hold the user space, and status
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674487742)
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 dangerous space to design, and we should hold the user space, and status
...
🤔 glozow reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1573365456)
> I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented.
Yeah the purpose of writing these tests was largely documentation and not changing behavior unintentionally.
I think most of the things here are intentional, though can think of at least one thing that seems to be "that's just how it got implemented." I was trying to figure out whether we would want to use a `notfound` for a parent tx
...
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1573365456)
> I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented.
Yeah the purpose of writing these tests was largely documentation and not changing behavior unintentionally.
I think most of the things here are intentional, though can think of at least one thing that seems to be "that's just how it got implemented." I was trying to figure out whether we would want to use a `notfound` for a parent tx
...
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291163628)
I'd be on board with that approach, too. A bit more explicit and reduces implementation complexity, while I think usability is as good, since we already require the user to distinguish between the arg having a default or not.
nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291163628)
I'd be on board with that approach, too. A bit more explicit and reduces implementation complexity, while I think usability is as good, since we already require the user to distinguish between the arg having a default or not.
nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.
💬 dergoegge commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674561856)
@ajtowns Thanks for the quick feedback! I think we'll mostly disagree here but I appreciate your opinion.
>> Most of the API simply shifts from CConnman::DoX(CNode*) to CConnman::DoX(NodeId)
>
> That seems undesirable to me? Where possible, acting directly on the node via a pointer/reference is much better than taking a global lock, doing a lookup, and then acting.
While it is obviously true that directly accessing the CNode pointers is more performant (no locking or look ups), accessing
...
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674561856)
@ajtowns Thanks for the quick feedback! I think we'll mostly disagree here but I appreciate your opinion.
>> Most of the API simply shifts from CConnman::DoX(CNode*) to CConnman::DoX(NodeId)
>
> That seems undesirable to me? Where possible, acting directly on the node via a pointer/reference is much better than taking a global lock, doing a lookup, and then acting.
While it is obviously true that directly accessing the CNode pointers is more performant (no locking or look ups), accessing
...
💬 ajtowns commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674592289)
> While it is obviously true that directly accessing the CNode pointers is more performant (no locking or look ups), accessing internals of other components like this leads to spaghetti code and the performance win of micro or nano seconds is just not worth it.
If we want to have multiple threads processing different peers, then avoiding (or at least minimising) having to go through shared data structures will be necessary. Keeping our data structures in sync is already challenging, making it
...
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674592289)
> While it is obviously true that directly accessing the CNode pointers is more performant (no locking or look ups), accessing internals of other components like this leads to spaghetti code and the performance win of micro or nano seconds is just not worth it.
If we want to have multiple threads processing different peers, then avoiding (or at least minimising) having to go through shared data structures will be necessary. Keeping our data structures in sync is already challenging, making it
...
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1674606177)
> Thanks, so it appears there are actually two mostly-unrelated network buffer deadlock issues, and Martin's test is likely triggering both of them.
So far, I haven't been able to trigger the original deadlock issue in my test when I run it on master - only the other one described above.
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1674606177)
> Thanks, so it appears there are actually two mostly-unrelated network buffer deadlock issues, and Martin's test is likely triggering both of them.
So far, I haven't been able to trigger the original deadlock issue in my test when I run it on master - only the other one described above.
💬 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
...