💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1477694019)
The linter started [complaining](https://cirrus-ci.com/task/5633626411368448?logs=lint#L224) about "E275 missing whitespace after keyword". Fixed this up in the individual commits.
ac438b10918ef05df2e564734d879699c8aa18f9 -> 6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae ([2022-05-connection-tracepoints.pr.1](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.1) -> [2022-05-connection-tracepoints.pr.2](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.2)
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1477694019)
The linter started [complaining](https://cirrus-ci.com/task/5633626411368448?logs=lint#L224) about "E275 missing whitespace after keyword". Fixed this up in the individual commits.
ac438b10918ef05df2e564734d879699c8aa18f9 -> 6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae ([2022-05-connection-tracepoints.pr.1](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.1) -> [2022-05-connection-tracepoints.pr.2](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.2)
...
💬 fanquake commented on issue "test: `wallet_importdescriptors.py --descriptors` failure":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477699527)
See also https://gist.github.com/fanquake/a458badc73abb47f8c06f009d15e1916 (combined log).
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477699527)
See also https://gist.github.com/fanquake/a458badc73abb47f8c06f009d15e1916 (combined log).
💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477705117)
Bitcoin Core has had fee estimation logic for a long time (`estimatefee` RPC was added in 2014, replaced with `estimatesmartfee` in 2015).
The Bitcoin Core wallet automatically uses this internal feerate estimation logic to decide on the fees of transactions. You need to override things, or use more low-level RPCs, to not use it.
That feerate estimation logic works by looking at how quickly mempool transactions get confirmed, over longer time windows, not by looking at the current mempool comp
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477705117)
Bitcoin Core has had fee estimation logic for a long time (`estimatefee` RPC was added in 2014, replaced with `estimatesmartfee` in 2015).
The Bitcoin Core wallet automatically uses this internal feerate estimation logic to decide on the fees of transactions. You need to override things, or use more low-level RPCs, to not use it.
That feerate estimation logic works by looking at how quickly mempool transactions get confirmed, over longer time windows, not by looking at the current mempool comp
...
📝 fanquake locked a pull request: "Year update"
(https://github.com/bitcoin/bitcoin/pull/27240)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27240)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477712121)
> 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 we merge this chang
...
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477712121)
> 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 we merge this chang
...
📝 fanquake locked a pull request: "Updated copyright years"
(https://github.com/bitcoin/bitcoin/pull/27267)
Not sure if this is a thing that should be done, but I noticed that alot of the copyright years are set in the past for the comments.
EDIT:
I did not find any mention about copyright dates and how these should be handled in the https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md file.
(https://github.com/bitcoin/bitcoin/pull/27267)
Not sure if this is a thing that should be done, but I noticed that alot of the copyright years are set in the past for the comments.
EDIT:
I did not find any mention about copyright dates and how these should be handled in the https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md file.
📝 fanquake locked a pull request: "build: ignore common editor files"
(https://github.com/bitcoin/bitcoin/pull/27275)
Add IntelliJ and Visual Studio Code editor files to .gitignore. Small QOL change :)
(https://github.com/bitcoin/bitcoin/pull/27275)
Add IntelliJ and Visual Studio Code editor files to .gitignore. Small QOL change :)
💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1143274477)
thinking about this a bit more, I think we should have a more robust way of checking this. iirc, taproot would be `tb1p` on testnet, so this would not work for segwit v1 addresses.
Maybe instead you could do something like `version, payload = bech32_to_bytes` and then check the version. If the version is None, then move on to `payload, version = base58_to_byte` etc.
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1143274477)
thinking about this a bit more, I think we should have a more robust way of checking this. iirc, taproot would be `tb1p` on testnet, so this would not work for segwit v1 addresses.
Maybe instead you could do something like `version, payload = bech32_to_bytes` and then check the version. If the version is None, then move on to `payload, version = base58_to_byte` etc.
💬 furszy commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477731516)
> > 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 we merge
...
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477731516)
> > 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 we merge
...
💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143232308)
nit: it's actually an argument, but I would just leave it out
```suggestion
* if the key is not found or if uri_parsed is nullptr (most probably from a failed parsing).
```
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143232308)
nit: it's actually an argument, but I would just leave it out
```suggestion
* if the key is not found or if uri_parsed is nullptr (most probably from a failed parsing).
```
💬 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.