💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098532940)
I think the original bug here illustrates that the enum is not well suited for this. Definitely not with a value of 0 for NO_LIMIT anyway. Got any other ideas? :)
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098532940)
I think the original bug here illustrates that the enum is not well suited for this. Definitely not with a value of 0 for NO_LIMIT anyway. Got any other ideas? :)
🤔 ryanofsky reviewed a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944)
Code review a885a166cec6d84d08600f12b25d912bd28af80e. This is good, change, but I think it has a few issues pointed out in comments below. At a high level I think it makes sense to decouple ECC initialization from the kernel context struct, but I don't think it makes sense to couple it so tightly with the node context struct.
Would suggest an approach more like the this branch: [pr/ecc](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) with commits:
- c6b4b7c050393b4442f155d40c2a7f586eb
...
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944)
Code review a885a166cec6d84d08600f12b25d912bd28af80e. This is good, change, but I think it has a few issues pointed out in comments below. At a high level I think it makes sense to decouple ECC initialization from the kernel context struct, but I don't think it makes sense to couple it so tightly with the node context struct.
Would suggest an approach more like the this branch: [pr/ecc](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) with commits:
- c6b4b7c050393b4442f155d40c2a7f586eb
...
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592362999)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
Is there a reason to suppress the lint warning instead of just deleting the destructor, since the destructor does not do anything? If the destructor is needed for some reason, it might be good to clarify in a comment.
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592362999)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
Is there a reason to suppress the lint warning instead of just deleting the destructor, since the destructor does not do anything? If the destructor is needed for some reason, it might be good to clarify in a comment.
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592383067)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
Not directly related to this PR, but I think the code comment above is basically out of date. Initial idea for `kernel::Context` was to have it hold a collection of variables that kernel code needed to run, so kernel code could use the context struct instead of global variables, and application code would still be convenient to write.
But this was before Options structs were introduced, and
...
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592383067)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
Not directly related to this PR, but I think the code comment above is basically out of date. Initial idea for `kernel::Context` was to have it hold a collection of variables that kernel code needed to run, so kernel code could use the context struct instead of global variables, and application code would still be convenient to write.
But this was before Options structs were introduced, and
...
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592585024)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
I don't think it's a great idea to add these ECC_Start / ECC_Stop calls here, because this prevents writing lighter-weight tests that may need to pass a `NodeContext` argument to RPC or Init functions, but don't need ECC. Also calling ECC_Start in the constructor makes it not possible for different NodeContext instances to be constructed at the same time. So for example you couldn't write a te
...
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592585024)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)
I don't think it's a great idea to add these ECC_Start / ECC_Stop calls here, because this prevents writing lighter-weight tests that may need to pass a `NodeContext` argument to RPC or Init functions, but don't need ECC. Also calling ECC_Start in the constructor makes it not possible for different NodeContext instances to be constructed at the same time. So for example you couldn't write a te
...
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592548893)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1591560587
> This is really unfortunate, but I can't think of anything less hack-ish. And at least it's relegated to the test code.
I debugged this, and the reason this is needed is that `node.kernel.reset()` calls during shutdown (in `Shutdown()` and `BasicTestingSetup::~BasicTestingSetup`) are no longer calling `ECC_Stop`. If those places are updated to still shut down ECC like before, this could go away.
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592548893)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1591560587
> This is really unfortunate, but I can't think of anything less hack-ish. And at least it's relegated to the test code.
I debugged this, and the reason this is needed is that `node.kernel.reset()` calls during shutdown (in `Shutdown()` and `BasicTestingSetup::~BasicTestingSetup`) are no longer calling `ECC_Stop`. If those places are updated to still shut down ECC like before, this could go away.
💬 Christewart commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1592626258)
How are users of the RPC expected to obtain `cluster_id`?
IIUC, the way to do this currently is to query
```
./bin/bitcoin-cli getrawmempool true
```
and then aggregate the `cluster_id`'s yourself?
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1592626258)
How are users of the RPC expected to obtain `cluster_id`?
IIUC, the way to do this currently is to query
```
./bin/bitcoin-cli getrawmempool true
```
and then aggregate the `cluster_id`'s yourself?
👋 TheCharlatan's pull request is ready for review: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252)
(https://github.com/bitcoin/bitcoin/pull/29252)
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098604388)
> I think the original bug here illustrates that the enum is not well suited for this. Definitely not with a value of 0 for NO_LIMIT anyway. Got any other ideas? :)
Yeah, I was getting an icky feeling while updating the PR for 2 reasons: `NO_LIMIT` of 0 is crappy and also the caller needs to make sure to use the same `CharLimit` for `Decode` and again with `LocateErrors`, which seems foot-gunny.
My ideal solution is `Decode`/`LocateErrors` shouldn't have a character limit at all (or use 10
...
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098604388)
> I think the original bug here illustrates that the enum is not well suited for this. Definitely not with a value of 0 for NO_LIMIT anyway. Got any other ideas? :)
Yeah, I was getting an icky feeling while updating the PR for 2 reasons: `NO_LIMIT` of 0 is crappy and also the caller needs to make sure to use the same `CharLimit` for `Decode` and again with `LocateErrors`, which seems foot-gunny.
My ideal solution is `Decode`/`LocateErrors` shouldn't have a character limit at all (or use 10
...
💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962)
> it might be good to clarify in a comment.
There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")
While there currently is no unique_ptr, it is possible that one will be re-added in the future?
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962)
> it might be good to clarify in a comment.
There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")
While there currently is no unique_ptr, it is possible that one will be re-added in the future?
💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592662009)
Thanks for the feedback @maflcko, I agree with it all, mostly.
That said it was quite painful to integrate it into `lint/test_runner`, mainly because the `mlc` tool will only accept a _single_ file/dir argument as the target but will permit multiple `--exclude-path` options. Therefore the only way I could concieve of not linting random directories in the root src dir was to deduplicate the output of `git ls-tree -rtd --format='%(path)' HEAD ./` with all top-level directories found by `fs::rea
...
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592662009)
Thanks for the feedback @maflcko, I agree with it all, mostly.
That said it was quite painful to integrate it into `lint/test_runner`, mainly because the `mlc` tool will only accept a _single_ file/dir argument as the target but will permit multiple `--exclude-path` options. Therefore the only way I could concieve of not linting random directories in the root src dir was to deduplicate the output of `git ls-tree -rtd --format='%(path)' HEAD ./` with all top-level directories found by `fs::rea
...
👍 alfonsoromanz approved a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2043495326)
ACK ee67bba76cca2355541f99bb731f58479981b29e. Code looks good to me. I also ran `test/functional/feature_assumeutxo.py` to make sure all tests passes, including this one.
<img width="943" alt="Screenshot 2024-05-07 at 12 07 04" src="https://github.com/bitcoin/bitcoin/assets/19962151/bbe9ced1-443d-43cc-9111-0e61e35fc7d6">
<img width="876" alt="Screenshot 2024-05-07 at 12 07 18" src="https://github.com/bitcoin/bitcoin/assets/19962151/36e2e6fb-39fa-4ec1-b25d-5247d7ded24d">
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2043495326)
ACK ee67bba76cca2355541f99bb731f58479981b29e. Code looks good to me. I also ran `test/functional/feature_assumeutxo.py` to make sure all tests passes, including this one.
<img width="943" alt="Screenshot 2024-05-07 at 12 07 04" src="https://github.com/bitcoin/bitcoin/assets/19962151/bbe9ced1-443d-43cc-9111-0e61e35fc7d6">
<img width="876" alt="Screenshot 2024-05-07 at 12 07 18" src="https://github.com/bitcoin/bitcoin/assets/19962151/36e2e6fb-39fa-4ec1-b25d-5247d7ded24d">
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592676778)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962
> There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")
>
> While there currently is no unique_ptr, it is possible that one will be re-added in the future?
Oh interesting. I was wondering why NOLINT was needed here but not in NodeContext, but I guess it is because NodeContext does contain unique_ptr members, and this doesn't
...
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592676778)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592642962
> There is already a comment in line 19, which explains why this is done ("should be stored to std::unique_ptr members pointing to opaque types.")
>
> While there currently is no unique_ptr, it is possible that one will be re-added in the future?
Oh interesting. I was wondering why NOLINT was needed here but not in NodeContext, but I guess it is because NodeContext does contain unique_ptr members, and this doesn't
...
💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592685484)
> Therefore the only way I could concieve of not linting random directories in the root src dir
If this is too difficult, it does not need to be implemented for now. The risk should be minimal, as `mlc` should only have read-only access? And if it were to fail for someone that put random other folders into the root, they can always skip the check?
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592685484)
> Therefore the only way I could concieve of not linting random directories in the root src dir
If this is too difficult, it does not need to be implemented for now. The risk should be minimal, as `mlc` should only have read-only access? And if it were to fail for someone that put random other folders into the root, they can always skip the check?
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864)
I set `dstaddr` and `dstport` here in `connection_made()` because when accepting connections (from the python node point of view) they are set to `"0"` and `0` respectively (in `master`):
https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203
https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186
When I add those asserts I get:
```
assert s
...
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864)
I set `dstaddr` and `dstport` here in `connection_made()` because when accepting connections (from the python node point of view) they are set to `"0"` and `0` respectively (in `master`):
https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203
https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186
When I add those asserts I get:
```
assert s
...
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592706360)
No, I just picked some value that looks reasonable, anything would work. For example `1` or `5` would still work but does not look reasonable because it will cause a lot of iteration loops.
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592706360)
No, I just picked some value that looks reasonable, anything would work. For example `1` or `5` would still work but does not look reasonable because it will cause a lot of iteration loops.
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592709053)
I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592709053)
I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).
💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592709785)
Yes, I think this is probably the best approach too. I'll make those changes and push here soon (tm)
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592709785)
Yes, I think this is probably the best approach too. I'll make those changes and push here soon (tm)
💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1592709382)
Is there a reason to name it something different here? Especially while the option is still broken and doesn't actually limit datacarriers?
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1592709382)
Is there a reason to name it something different here? Especially while the option is still broken and doesn't actually limit datacarriers?
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2098816463)
> [clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
Same for the non-mocked one:
[clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)
> [clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
Same for the non-mocked one:
[clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)
![Screenshot from 2024-05-07 18-05-00](https://github.com/
...