✅ PiRK closed a pull request: "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage"
(https://github.com/bitcoin/bitcoin/pull/33381)
(https://github.com/bitcoin/bitcoin/pull/33381)
💬 l0rinc commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423544412)
I'm not sure I understand - why would we do anything like this? The LLM is just a tool, it would be weird to say: "suggestions by: clang-tidy" or "IDE used: CLion" or "Assisted-by: The C++ Programming Language: Special Edition".
Maybe the problem is committing something here that's "substantially generated with automated assistance" - in the same sense as we would discourage copy-pasting something here from StackOverflow without tailoring it to our needs.
So that's a concept NACK from me.
...
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423544412)
I'm not sure I understand - why would we do anything like this? The LLM is just a tool, it would be weird to say: "suggestions by: clang-tidy" or "IDE used: CLion" or "Assisted-by: The C++ Programming Language: Special Edition".
Maybe the problem is committing something here that's "substantially generated with automated assistance" - in the same sense as we would discourage copy-pasting something here from StackOverflow without tailoring it to our needs.
So that's a concept NACK from me.
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2445998747)
Agreed, though I think this type of behavior change regarding what packages we accept can happen in a separate PR after cluster mempool.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2445998747)
Agreed, though I think this type of behavior change regarding what packages we accept can happen in a separate PR after cluster mempool.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446013049)
I have used the same implementation for both endpoints for simplicity and to avoid code duplication.
I can add an additional argument to enable `offset` and `size` parameter handling for the `/rest/blockpart` endpoint (and disable it for `/rest/block` endpoint).
WDYT?
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446013049)
I have used the same implementation for both endpoints for simplicity and to avoid code duplication.
I can add an additional argument to enable `offset` and `size` parameter handling for the `/rest/blockpart` endpoint (and disable it for `/rest/block` endpoint).
WDYT?
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2446016715)
I'm inclined to implement your suggestion, so that any cluster size limit failures are treated as non-RECONSIDERABLE for now. TRUC transactions would never hit these cluster limits, so users who would be concerned about cluster size limits as a potential pinning vector for CPFP packages already have a mechanism to avoid it.
(I also considered having all cluster size limits being treated as RECONSIDERABLE, but decided that is also not great, partly because we certainly shouldn't let a transac
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2446016715)
I'm inclined to implement your suggestion, so that any cluster size limit failures are treated as non-RECONSIDERABLE for now. TRUC transactions would never hit these cluster limits, so users who would be concerned about cluster size limits as a potential pinning vector for CPFP packages already have a mechanism to avoid it.
(I also considered having all cluster size limits being treated as RECONSIDERABLE, but decided that is also not great, partly because we certainly shouldn't let a transac
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446030515)
Sounds good - fixed in https://github.com/bitcoin/bitcoin/compare/787d6df48af31b42ee9a69ff5c33e73bbe8cf8e9..c4bf7a77c45e297672222a2e1a984ef8d755b471.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446030515)
Sounds good - fixed in https://github.com/bitcoin/bitcoin/compare/787d6df48af31b42ee9a69ff5c33e73bbe8cf8e9..c4bf7a77c45e297672222a2e1a984ef8d755b471.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446030635)
Thanks - fixed in https://github.com/bitcoin/bitcoin/compare/787d6df48af31b42ee9a69ff5c33e73bbe8cf8e9..c4bf7a77c45e297672222a2e1a984ef8d755b471.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446030635)
Thanks - fixed in https://github.com/bitcoin/bitcoin/compare/787d6df48af31b42ee9a69ff5c33e73bbe8cf8e9..c4bf7a77c45e297672222a2e1a984ef8d755b471.
💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446031471)
Seems we were wrong, the test assumes that after this call the given `COutPoint` will correspond to the current coins - which isn't the case with `EmplaceCoinInternalDANGER` which never replaces, so I have reverted https://github.com/bitcoin/bitcoin/compare/62cb2a2ed84e695c0959d8e27fc5bedd9b5a135e..54151d612b63f4e42fe03a143540e597728a4cc3
See the previous errors that the suggestions resulted in:
https://github.com/bitcoin/bitcoin/actions/runs/18611576758/job/53070427535?pr=33512#step:7:40482
...
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446031471)
Seems we were wrong, the test assumes that after this call the given `COutPoint` will correspond to the current coins - which isn't the case with `EmplaceCoinInternalDANGER` which never replaces, so I have reverted https://github.com/bitcoin/bitcoin/compare/62cb2a2ed84e695c0959d8e27fc5bedd9b5a135e..54151d612b63f4e42fe03a143540e597728a4cc3
See the previous errors that the suggestions resulted in:
https://github.com/bitcoin/bitcoin/actions/runs/18611576758/job/53070427535?pr=33512#step:7:40482
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3423601774)
> Some of the PR description is reused from the previous PR, but is now stale.
Good catch, thanks!
Updated PR description: https://github.com/bitcoin/bitcoin/pull/33657#issue-3529967048
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3423601774)
> Some of the PR description is reused from the previous PR, but is now stale.
Good catch, thanks!
Updated PR description: https://github.com/bitcoin/bitcoin/pull/33657#issue-3529967048
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2446074332)
The latest https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446 indicates that `SingletonClusterImpl::Split` may not be covered fully.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2446074332)
The latest https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446 indicates that `SingletonClusterImpl::Split` may not be covered fully.
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3423668761)
Concept ACK, thanks for helping with the coverage and documentation.
For reference https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/chain.cpp.gcov.html is pretty well covered otherwise, but these tests can help with the specifics.
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3423668761)
Concept ACK, thanks for helping with the coverage and documentation.
For reference https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/chain.cpp.gcov.html is pretty well covered otherwise, but these tests can help with the specifics.
💬 kanzure commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423684598)
Using a canary is a clever idea. I think I agree with @l0rinc's reasoning that this is not a good idea, but if there was a decision to insert an attempt at a LLM canary then I would suggest double checking the models and see if offering them a few dollars to donate to a philanthropic cause is more likely to evoke the LLM behavior you are trying to get. This strategy was previously effective, no idea if it's still effective.
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423684598)
Using a canary is a clever idea. I think I agree with @l0rinc's reasoning that this is not a good idea, but if there was a decision to insert an attempt at a LLM canary then I would suggest double checking the models and see if offering them a few dollars to donate to a philanthropic cause is more likely to evoke the LLM behavior you are trying to get. This strategy was previously effective, no idea if it's still effective.
💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446168275)
If I understand correctly, this would allow passing `offset` or `size` to `/rest/block` along with `.json` format specification. Normally this would fail to deserialize, but if very unlucky it could deserialize to what looks like a block and return? That would be very bad.
Similarly if not using `.json`, this would return incorrect data that a user might expect (for instance if they switched rest endpoints but forgot to remove parameters).
I would recommend disabling these parameters if not
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446168275)
If I understand correctly, this would allow passing `offset` or `size` to `/rest/block` along with `.json` format specification. Normally this would fail to deserialize, but if very unlucky it could deserialize to what looks like a block and return? That would be very bad.
Similarly if not using `.json`, this would return incorrect data that a user might expect (for instance if they switched rest endpoints but forgot to remove parameters).
I would recommend disabling these parameters if not
...
✅ plebhash closed an issue: "[`v30.0`] `createNewBlock` never returns"
(https://github.com/bitcoin/bitcoin/issues/33647)
(https://github.com/bitcoin/bitcoin/issues/33647)
💬 plebhash commented on issue "[`v30.0`] `createNewBlock` never returns":
(https://github.com/bitcoin/bitcoin/issues/33647#issuecomment-3423804916)
running against `30.x` branch made it go away, thanks!
(https://github.com/bitcoin/bitcoin/issues/33647#issuecomment-3423804916)
running against `30.x` branch made it go away, thanks!
💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446263560)
We should probably cover this as well as other cases of `ReadRawBlockPart` in `blockmanager_tests.cpp`.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446263560)
We should probably cover this as well as other cases of `ReadRawBlockPart` in `blockmanager_tests.cpp`.
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446373422)
> the test assumes that after this call the given COutPoint will correspond to the current coins - which isn't the case with EmplaceCoinInternalDANGER which never replaces
Right, so we still need `!stack.back()->map().contains(op)` but we don't need the `!coin.IsSpent()` check. Also, a few lines up we randomly set the pubkey to unspendable. In `AddCoin` we check if it is unspendable and don't add it, but in `EmplaceCoinInternalDANGER` we would add it. So I think we should also not use `Emplac
...
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446373422)
> the test assumes that after this call the given COutPoint will correspond to the current coins - which isn't the case with EmplaceCoinInternalDANGER which never replaces
Right, so we still need `!stack.back()->map().contains(op)` but we don't need the `!coin.IsSpent()` check. Also, a few lines up we randomly set the pubkey to unspendable. In `AddCoin` we check if it is unspendable and don't add it, but in `EmplaceCoinInternalDANGER` we would add it. So I think we should also not use `Emplac
...
⚠️ linobadass1234-cmyk opened an issue: "d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3"
(https://github.com/bitcoin/bitcoin/issues/33664)
(https://github.com/bitcoin/bitcoin/issues/33664)
✅ linobadass1234-cmyk closed an issue: "d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3"
(https://github.com/bitcoin/bitcoin/issues/33664)
(https://github.com/bitcoin/bitcoin/issues/33664)
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446409517)
```suggestion
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !newcoin.out.scriptPubKey.IsUnspendable() && m_rng.randbool()) {
```
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446409517)
```suggestion
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !newcoin.out.scriptPubKey.IsUnspendable() && m_rng.randbool()) {
```