💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474401085)
In commit bc6788ab description:
"Adds options for wiping the chainstate and block tree indexes to the chainstate load options" -> "... chainstate manager options" (since only one is a load option)
Also for efcfe2df commit title and 2ecd926c commit description.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474401085)
In commit bc6788ab description:
"Adds options for wiping the chainstate and block tree indexes to the chainstate load options" -> "... chainstate manager options" (since only one is a load option)
Also for efcfe2df commit title and 2ecd926c commit description.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2478084857)
```suggestion
/**
* @brief Triggers the start of a reindex if the wipe options were previously set for
* the chainstate manager. Can also import an array of existing block
* files selected by the user.
*
* @param[in] chainstate_manager Non-null.
* @param[in] block_file_paths_data Nullable, array of block files described by their full filesystem paths.
* @param[in] block_file_paths_lens Nullable, array containing the lengths of each of the paths.
* @param[in] block
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2478084857)
```suggestion
/**
* @brief Triggers the start of a reindex if the wipe options were previously set for
* the chainstate manager. Can also import an array of existing block
* files selected by the user.
*
* @param[in] chainstate_manager Non-null.
* @param[in] block_file_paths_data Nullable, array of block files described by their full filesystem paths.
* @param[in] block_file_paths_lens Nullable, array containing the lengths of each of the paths.
* @param[in] block
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474412320)
It seems [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459840316) was not applied (force push [message](https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3387750815) suggests otherwise)?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474412320)
It seems [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459840316) was not applied (force push [message](https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3387750815) suggests otherwise)?
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2477740569)
In commit 2ecd926c description:
"import blocks from a single specified file path" -> "import blocks from specified file paths"
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2477740569)
In commit 2ecd926c description:
"import blocks from a single specified file path" -> "import blocks from specified file paths"
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2479513352)
Could this be also declared as final? (like `KernelValidationInterface`)
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2479513352)
Could this be also declared as final? (like `KernelValidationInterface`)
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481021568)
Is this line necessary?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481021568)
Is this line necessary?
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481753366)
Also related to the discussion [here](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481753366)
Also related to the discussion [here](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366).
💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2481763116)
> As far as I can tell, CTransaction's hashes are computed and cached on construction
No, I mean `uint256 hash = header.GetHash()` is only needed for the debug log - let me try separating all calculations that are only needed for debugging. Given that we're clearing `header` and `txn_available` I understand why the split wasn't done before. But we can still use `block` for the hash and `vtx_missing` for the missing, so I think it should be possible to split the two.
That's a more invasive ch
...
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2481763116)
> As far as I can tell, CTransaction's hashes are computed and cached on construction
No, I mean `uint256 hash = header.GetHash()` is only needed for the debug log - let me try separating all calculations that are only needed for debugging. Given that we're clearing `header` and `txn_available` I understand why the split wasn't done before. But we can still use `block` for the hash and `vtx_missing` for the missing, so I think it should be possible to split the two.
That's a more invasive ch
...
💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481787433)
I agree that `zmqDebug`/`zmqError` would be ideal, however `zmqError` is currently used to log at the `Debug` level - changing it to log at `Error` level while keeping the type signature the same seems likely to cause misuse.
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481787433)
I agree that `zmqDebug`/`zmqError` would be ideal, however `zmqError` is currently used to log at the `Debug` level - changing it to log at `Error` level while keeping the type signature the same seems likely to cause misuse.
💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481808137)
MAX_REST_HEADERS_RESULTS is a small number, so I don't think we need any architecture-specific and special PTRDIFF handling and can just use fixed size integral values.
Also, to walk backwards, I am sure you can just use `pprev` and not need to create a new method.
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481808137)
MAX_REST_HEADERS_RESULTS is a small number, so I don't think we need any architecture-specific and special PTRDIFF handling and can just use fixed size integral values.
Also, to walk backwards, I am sure you can just use `pprev` and not need to create a new method.
💬 instagibbs commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473633999)
If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473633999)
If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
🚀 fanquake merged a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753)
(https://github.com/bitcoin/bitcoin/pull/33753)
💬 fanquake commented on pull request "test: ipc: resolve symlinks in `which capnp`":
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3473673447)
cc @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3473673447)
cc @ryanofsky
🤔 Sjors reviewed a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3404815806)
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
I left some suggestions for extra clarity.
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3404815806)
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
I left some suggestions for extra clarity.
💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481810673)
Is this checking that the response to `waitNext()` is a null template?
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481810673)
Is this checking that the response to `waitNext()` is a null template?
💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481802226)
nit: you could move these helper methods out of the test:
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..7a447eba92 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -82,10 +82,20 @@ class IPCInterfaceTest(BitcoinTestFramework):
coinbase_data = BytesIO((await block_template.result.getCoinbaseTx(ctx)).result)
tx = CTransaction()
tx.deserialize(coinbase_data)
retur
...
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481802226)
nit: you could move these helper methods out of the test:
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..7a447eba92 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -82,10 +82,20 @@ class IPCInterfaceTest(BitcoinTestFramework):
coinbase_data = BytesIO((await block_template.result.getCoinbaseTx(ctx)).result)
tx = CTransaction()
tx.deserialize(coinbase_data)
retur
...
💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500)
It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line can check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500)
It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line can check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
💬 TheCharlatan commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473751931)
> If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
https://docs.counterparty.io/docs/advanced/protocol/#transactions and https://github.com/CounterpartyXCP/counterparty-core/blob/master/counterparty-core/counterpartycore/lib/api/composer.py#L304 should give an idea. AFAICT the real pubkey goes last.
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473751931)
> If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
https://docs.counterparty.io/docs/advanced/protocol/#transactions and https://github.com/CounterpartyXCP/counterparty-core/blob/master/counterparty-core/counterpartycore/lib/api/composer.py#L304 should give an idea. AFAICT the real pubkey goes last.
💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473753273)
Github completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473753273)
Github completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843
💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473764682)
@ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I've simply gotten used to how things were.
> The basic problem here is that this option's default value (ip_asn.map) and default behavi
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473764682)
@ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I've simply gotten used to how things were.
> The basic problem here is that this option's default value (ip_asn.map) and default behavi
...