💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143234062)
nit
```suggestion
* @param[in] uri_parsed is the uri parsed with libevent's evhttp_uri_parse
```
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143234062)
nit
```suggestion
* @param[in] uri_parsed is the uri parsed with libevent's evhttp_uri_parse
```
💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143273441)
I think the tests are still more complex than necessary. How about this:
<details>
<summary>git diff</summary>
```diff
diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index 0462d3ae3..6ae57bdf0 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -11,42 +11,46 @@
BOOST_FIXTURE_TEST_SUITE(httpserver_tests, BasicTestingSetup)
-static evhttp_uri* GetURIParsed(const std::string uri){
- return evhttp_uri_parse(uri.c_str(
...
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143273441)
I think the tests are still more complex than necessary. How about this:
<details>
<summary>git diff</summary>
```diff
diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index 0462d3ae3..6ae57bdf0 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -11,42 +11,46 @@
BOOST_FIXTURE_TEST_SUITE(httpserver_tests, BasicTestingSetup)
-static evhttp_uri* GetURIParsed(const std::string uri){
- return evhttp_uri_parse(uri.c_str(
...
👍 willcl-ark approved a pull request: "refactor: Replace GetTimeMicros by SystemClock"
(https://github.com/bitcoin/bitcoin/pull/27233)
tACK faf3f1242
As I was unfamiliar with this code it was a little counter-intuitive to me at first that we truncate the time to seconds, before re-adding the microseconds if `m_log_time_micros` was set, but sure enough it mirrors the old behaviour.
I also left one tiny nit/comment which doesn't need to be addressed.
(https://github.com/bitcoin/bitcoin/pull/27233)
tACK faf3f1242
As I was unfamiliar with this code it was a little counter-intuitive to me at first that we truncate the time to seconds, before re-adding the microseconds if `m_log_time_micros` was set, but sure enough it mirrors the old behaviour.
I also left one tiny nit/comment which doesn't need to be addressed.
💬 willcl-ark commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143268447)
nit: as we only use this in one single place (_logging.cpp_) it almost seems to make more sense just to drop this and directly use `std::chrono::system_clock` there? Although having `SteadyClock` and `SystemClock` as the two options perhaps prefereable.
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143268447)
nit: as we only use this in one single place (_logging.cpp_) it almost seems to make more sense just to drop this and directly use `std::chrono::system_clock` there? Although having `SteadyClock` and `SystemClock` as the two options perhaps prefereable.
💬 MarcoFalke commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143290208)
If this is used in more places in the future, it seems convenient to have `NodeClock`, `SteadyClock`, and `SystemClock` in the same header?
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143290208)
If this is used in more places in the future, it seems convenient to have `NodeClock`, `SteadyClock`, and `SystemClock` in the same header?
💬 dergoegge commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1143291078)
Thinking a little bit about what data should go into the tracepoints.
`m_addr_name`: would it make more sense to put `addr` in binary form (or both)? I'm not sure they are equivalent all the time.
`nKeyedNetGroup`: is derived deterministically from `addr` using random seeds that are created with every `CConnman`. I think that means that across restarts of a node previous net groups loose meaning (i.e. you can't compare them to netgroups of new connections). Maybe `NetGroupManager::GetGroup(a
...
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1143291078)
Thinking a little bit about what data should go into the tracepoints.
`m_addr_name`: would it make more sense to put `addr` in binary form (or both)? I'm not sure they are equivalent all the time.
`nKeyedNetGroup`: is derived deterministically from `addr` using random seeds that are created with every `CConnman`. I think that means that across restarts of a node previous net groups loose meaning (i.e. you can't compare them to netgroups of new connections). Maybe `NetGroupManager::GetGroup(a
...
💬 Sjors commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477744971)
It turns out 82e9f4d70b51f130446442f5fbde03413c9fd076 triggers the test failure I [described above](https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476364655), but it had no impact on the wallet loading / unloading. So maybe it's not exactly the same issue.
Starting with `-privdb=0` indeed removes the memory leak for me for wallet loading / unloading. Dropping the `DB_PRIVATE` in `bdb.cpp` also fixes the test for me.
The release notes for 0.3.13 state "Dropped DB_PRIVATE Berk
...
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477744971)
It turns out 82e9f4d70b51f130446442f5fbde03413c9fd076 triggers the test failure I [described above](https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476364655), but it had no impact on the wallet loading / unloading. So maybe it's not exactly the same issue.
Starting with `-privdb=0` indeed removes the memory leak for me for wallet loading / unloading. Dropping the `DB_PRIVATE` in `bdb.cpp` also fixes the test for me.
The release notes for 0.3.13 state "Dropped DB_PRIVATE Berk
...
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477751846)
Updated c7af97113459f50cf33882fa18909a6109e914f4 -> a87002550c1461a4cfbe666d5034597276e32639 ([pr26749.14](https://github.com/hebasto/bitcoin/commits/pr26749.14) -> [pr26749.15](https://github.com/hebasto/bitcoin/commits/pr26749.15)):
- rebased to avoid a silent conflict with #26705
- added a new commit "_clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck`_"
@MarcoFalke
> Might be good to keep the clang-tidy related change in a separate commit, because it is independent
...
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477751846)
Updated c7af97113459f50cf33882fa18909a6109e914f4 -> a87002550c1461a4cfbe666d5034597276e32639 ([pr26749.14](https://github.com/hebasto/bitcoin/commits/pr26749.14) -> [pr26749.15](https://github.com/hebasto/bitcoin/commits/pr26749.15)):
- rebased to avoid a silent conflict with #26705
- added a new commit "_clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck`_"
@MarcoFalke
> Might be good to keep the clang-tidy related change in a separate commit, because it is independent
...
💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143292320)
I'm not sure that it is guaranteed that the capacity is actually the same after reserve(), as far as I know it can by anything that's at least as large as reserve(). I think with stdlibc++ it's correct, but no idea what e.g. microsoft is doing. So I'd just leave this as InsecureRandRange(10)
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143292320)
I'm not sure that it is guaranteed that the capacity is actually the same after reserve(), as far as I know it can by anything that's at least as large as reserve(). I think with stdlibc++ it's correct, but no idea what e.g. microsoft is doing. So I'd just leave this as InsecureRandRange(10)
💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143303982)
The standard doesn't seem to say anything else, so the real allocation capacity seems to be implementation defined: https://eel.is/c++draft/vector#capacity-4
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143303982)
The standard doesn't seem to say anything else, so the real allocation capacity seems to be implementation defined: https://eel.is/c++draft/vector#capacity-4
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477759517)
> Like https://github.com/bitcoin/bitcoin/commit/06f5f3931910dcd2056167b1d440d327f3a510b0?
I was referring to the `bugprone-use-after-move` workaround.
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477759517)
> Like https://github.com/bitcoin/bitcoin/commit/06f5f3931910dcd2056167b1d440d327f3a510b0?
I was referring to the `bugprone-use-after-move` workaround.
💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477762451)
> > > Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.
> >
> >
> > ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).
> > Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend
...
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477762451)
> > > Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.
> >
> >
> > ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).
> > Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend
...
💬 furszy commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477783857)
k sure. I didn't suggest that because the error message will make people select the input/s manually etc. Which, I'm not so sure that it's better than a crash (at least the crash prevent them from making mistakes).
But.. I'm not that strong over that point. So, all good, either way works for me. The coin selection fixes are already up anyway, matter of continue moving forward until all the problems are corrected, so this can be enabled.
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477783857)
k sure. I didn't suggest that because the error message will make people select the input/s manually etc. Which, I'm not so sure that it's better than a crash (at least the crash prevent them from making mistakes).
But.. I'm not that strong over that point. So, all good, either way works for me. The coin selection fixes are already up anyway, matter of continue moving forward until all the problems are corrected, so this can be enabled.
💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477800508)
Cool! Seeing a **~8% speedup** over a modern region of the chain with **lower memory usage**.
---
| bench name | command
...
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477800508)
Cool! Seeing a **~8% speedup** over a modern region of the chain with **lower memory usage**.
---
| bench name | command
...
💬 MarcoFalke commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .
See also https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1409699173
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .
See also https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1409699173
📝 MarcoFalke opened a pull request: "Refactor: Remove unused FlatFilePos::SetNull"
(https://github.com/bitcoin/bitcoin/pull/27289)
This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor.
Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method.
(https://github.com/bitcoin/bitcoin/pull/27289)
This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor.
Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)
If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method.
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477809450)
Updated a87002550c1461a4cfbe666d5034597276e32639 -> 95ad70ab652ddde7de65f633c36c1378b26a313a ([pr26749.15](https://github.com/hebasto/bitcoin/commits/pr26749.15) -> [pr26749.16](https://github.com/hebasto/bitcoin/commits/pr26749.16), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.15..pr26749.16)):
- addressed @martinus's [comment](https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143292320)
- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26749#
...
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477809450)
Updated a87002550c1461a4cfbe666d5034597276e32639 -> 95ad70ab652ddde7de65f633c36c1378b26a313a ([pr26749.15](https://github.com/hebasto/bitcoin/commits/pr26749.15) -> [pr26749.16](https://github.com/hebasto/bitcoin/commits/pr26749.16), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.15..pr26749.16)):
- addressed @martinus's [comment](https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143292320)
- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26749#
...
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143347226)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477809450).
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143347226)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477809450).
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1477825604)
> Yikes. Work on adding [`cppcoreguidelines-pro-type-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html)` to clang-tidy maybe?
Well, it fires too many false warnings for our code base (for example, for `CTxOut::CTxOut()` constructor).
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1477825604)
> Yikes. Work on adding [`cppcoreguidelines-pro-type-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html)` to clang-tidy maybe?
Well, it fires too many false warnings for our code base (for example, for `CTxOut::CTxOut()` constructor).
💬 ajtowns commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1477834856)
> Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?
I've added logging for `TX_NOT_STANDARD` in `MaybePunishNodeForTx`. After 20 hours or so running that on a listening node, I've had a couple of instances of rejections for `dust` (via `IsStandardTx` for outputs matching `IsDust`), but so far that's it.
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1477834856)
> Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?
I've added logging for `TX_NOT_STANDARD` in `MaybePunishNodeForTx`. After 20 hours or so running that on a listening node, I've had a couple of instances of rejections for `dust` (via `IsStandardTx` for outputs matching `IsDust`), but so far that's it.