💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785527518)
edited
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785527518)
edited
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656263)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656263)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656274)
fixed, thanks
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656274)
fixed, thanks
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656312)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656312)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656318)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656318)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656284)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656284)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785529982)
added
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1785529982)
added
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656296)
fixed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656296)
fixed
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656350)
Accidental, I think. changed back
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1788656350)
Accidental, I think. changed back
💬 hodlinator commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788683926)
How come the shallow copy in the version before the PR is causing a problem in this case though?
It seems the default-generated copy-ctor for `Node` used before was somehow ending up with corrupt/leaked data, but I've been unable to spot what it is. Can't see any slicing going on. Is something funky being done to the `shared_ptr`s?
Regardless, it might be worth adding:
```
Node(const Node&) = delete;
Node(Node&&) = delete;
Node& operator=(const Node&) = delete;
Node& o
...
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788683926)
How come the shallow copy in the version before the PR is causing a problem in this case though?
It seems the default-generated copy-ctor for `Node` used before was somehow ending up with corrupt/leaked data, but I've been unable to spot what it is. Can't see any slicing going on. Is something funky being done to the `shared_ptr`s?
Regardless, it might be worth adding:
```
Node(const Node&) = delete;
Node(Node&&) = delete;
Node& operator=(const Node&) = delete;
Node& o
...
💬 hodlinator commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395195267)
Having a formal spec, either [generated similarly to the current RPC docs](https://github.com/bitcoin-core/bitcoincore.org/blob/master/contrib/doc-gen/generate.go) (or possibly used to generate stub headers which are then implemented in Bitcoin Core) lowers friction for application developers building on top of Bitcoin Core and reduces temptation to build on top of other projects (including non-Bitcoin ones).
Been digging into OpenRPC a bit. It seems to be used primarily by Ethereum and Ether
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395195267)
Having a formal spec, either [generated similarly to the current RPC docs](https://github.com/bitcoin-core/bitcoincore.org/blob/master/contrib/doc-gen/generate.go) (or possibly used to generate stub headers which are then implemented in Bitcoin Core) lowers friction for application developers building on top of Bitcoin Core and reduces temptation to build on top of other projects (including non-Bitcoin ones).
Been digging into OpenRPC a bit. It seems to be used primarily by Ethereum and Ether
...
📝 hodlinator opened a pull request: "test: Fix copy-paste in wallet/test/db_tests ostream operator"
(https://github.com/bitcoin/bitcoin/pull/31038)
Fix accidentally remaining copy-pasted variable name.
Example output when intentionally adding `expected.erase(expected.begin());` before `BOOST_CHECK_EQUAL_COLLECTIONS` in *db_tests.cpp*/`CheckPrefix`
Before fix:
```
src/wallet/test/db_tests.cpp(61): error: in "db_tests/db_cursor_prefix_byte_test": check { actual.begin(), actual.end() } == { expected.begin(), expected.end() } has failed.
Mismatch at position 0: ("�", "�") != ("�suffix", "�suffix")
Mismatch at position 1: ("�suffix",
...
(https://github.com/bitcoin/bitcoin/pull/31038)
Fix accidentally remaining copy-pasted variable name.
Example output when intentionally adding `expected.erase(expected.begin());` before `BOOST_CHECK_EQUAL_COLLECTIONS` in *db_tests.cpp*/`CheckPrefix`
Before fix:
```
src/wallet/test/db_tests.cpp(61): error: in "db_tests/db_cursor_prefix_byte_test": check { actual.begin(), actual.end() } == { expected.begin(), expected.end() } has failed.
Mismatch at position 0: ("�", "�") != ("�suffix", "�suffix")
Mismatch at position 1: ("�suffix",
...
🤔 hodlinator reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2350404241)
`git range-diff master 1ba225c ded1a6c`
Still feel like it would be good to call a common function adding *-testdatadir* to an `ArgsManager` instead of duplicating the functionality between test/bench (and having a different description-string).
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2350404241)
`git range-diff master 1ba225c ded1a6c`
Still feel like it would be good to call a common function adding *-testdatadir* to an `ArgsManager` instead of duplicating the functionality between test/bench (and having a different description-string).
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788721681)
Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788721681)
Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788722245)
> I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
(`SetupBenchArgs` is capitalized, `main` must be non-capitalized, so that only leaves `parseAsymptote` to change beyond the function under discussion).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788722245)
> I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
(`SetupBenchArgs` is capitalized, `main` must be non-capitalized, so that only leaves `parseAsymptote` to change beyond the function under discussion).
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788724651)
A bit heavy for one arg as it is right now, but I like that it avoids repeating the name. :+1:
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788724651)
A bit heavy for one arg as it is right now, but I like that it avoids repeating the name. :+1:
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788760477)
> Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?
I'm not so in favor of mentioning upper level binaries or frameworks in lower level files but no problem in applying the suggestion.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788760477)
> Agree a `SetupTestArgs` may not communicate clearly enough that the function is used in a broader context. How about calling it `SetupCommonTestArgs` and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function `static` (private to the compilation unit), might mitigate your concerns?
I'm not so in favor of mentioning upper level binaries or frameworks in lower level files but no problem in applying the suggestion.
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788763177)
Hmm well, I'm still a bit reluctant to it. It would add `test/util/setup_common.h` dependency to the benchmark binary entry point just for a single line of code.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1788763177)
Hmm well, I'm still a bit reluctant to it. It would add `test/util/setup_common.h` dependency to the benchmark binary entry point just for a single line of code.
💬 kilianmh commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395240266)
Thank you for your comments @hodlinator
1.
> A more automated way would be to do something like the Golang RPC doc generator in my first link above.
Indeed, autogeneration is the best way forward. I just did copy paste to showcase what can be done.
2.
> Conversely, have you noticed anything bitcoind does that goes against the OpenRPC spec?
You mentioned the main thing that i am not sure of if it is possible:
> JSON-RPC interface only uses [one main path + wallet path](https
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2395240266)
Thank you for your comments @hodlinator
1.
> A more automated way would be to do something like the Golang RPC doc generator in my first link above.
Indeed, autogeneration is the best way forward. I just did copy paste to showcase what can be done.
2.
> Conversely, have you noticed anything bitcoind does that goes against the OpenRPC spec?
You mentioned the main thing that i am not sure of if it is possible:
> JSON-RPC interface only uses [one main path + wallet path](https
...
💬 1440000bytes commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2395248735)
There are 7000 IPv4 and 2000 IPv6 nodes. I expect this ratio to continue for next 10 years based on this [adoption](https://www.google.com/intl/en/ipv6/statistics.html#tab=per-country-ipv6-adoption).
IPv4 nodes will not be able to benefit from non-default port support because of DNS seeds and IPv6.
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2395248735)
There are 7000 IPv4 and 2000 IPv6 nodes. I expect this ratio to continue for next 10 years based on this [adoption](https://www.google.com/intl/en/ipv6/statistics.html#tab=per-country-ipv6-adoption).
IPv4 nodes will not be able to benefit from non-default port support because of DNS seeds and IPv6.