💬 jonatack commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476698491)
> I should have tested that, because it actually _does_ print `[validation:warning]` in the log.
>
> Maybe we should just stick to `fprintf` and change the TODO to say this should be `validation` level `info` once `LogPrintf` prints those by default.
Perhaps just go for `info`. Making info unconditionally logged is near the top of the next steps in the parent PR #25203 i.e. https://github.com/bitcoin/bitcoin/pull/25203/commits/118c7567f62df2b882877590f232242d7c627a05. Until then, -debug=va
...
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476698491)
> I should have tested that, because it actually _does_ print `[validation:warning]` in the log.
>
> Maybe we should just stick to `fprintf` and change the TODO to say this should be `validation` level `info` once `LogPrintf` prints those by default.
Perhaps just go for `info`. Making info unconditionally logged is near the top of the next steps in the parent PR #25203 i.e. https://github.com/bitcoin/bitcoin/pull/25203/commits/118c7567f62df2b882877590f232242d7c627a05. Until then, -debug=va
...
👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK a48010f1fd6d8a430b8234789ac3c91ec9d06972. Just comment and whitespace changes since last review
(https://github.com/bitcoin/bitcoin/pull/27217)
Code review ACK a48010f1fd6d8a430b8234789ac3c91ec9d06972. Just comment and whitespace changes since last review
💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476753978)
It's not limited to the test. Running that commit in regtest, creating a wallet and then exiting triggers it too. That should make debugging easier...
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476753978)
It's not limited to the test. Running that commit in regtest, creating a wallet and then exiting triggers it too. That should make debugging easier...
💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476761563)
Code review ACK ebfc2a2dddedcbf384f435716d30d8418a77505b and ran `git rebase --interactive upstream/master --exec 'make clean && make -j14 check'`. Btw. I really links to the changes in your PR updates. Do you use any script to help with that?
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476761563)
Code review ACK ebfc2a2dddedcbf384f435716d30d8418a77505b and ran `git rebase --interactive upstream/master --exec 'make clean && make -j14 check'`. Btw. I really links to the changes in your PR updates. Do you use any script to help with that?
👍 hebasto approved a pull request: "refactor: Extract util/fs from util/system"
(https://github.com/bitcoin/bitcoin/pull/27254)
ACK 3ceb8dde48fc12f4f16372f661a4cccf7104e194
The commit "_Add missing fs.h include..._" could address another IWYU [suggestion](https://api.cirrus-ci.com/v1/task/6080090677706752/logs/ci.log):
```diff
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -16,6 +16,7 @@
#include <tinyformat.h>
#include <txdb.h>
#include <uint256.h>
+#include "util/fs.h"
#include <util/time.h>
#include <util/translation.h>
#include <validation.h>
```
(https://github.com/bitcoin/bitcoin/pull/27254)
ACK 3ceb8dde48fc12f4f16372f661a4cccf7104e194
The commit "_Add missing fs.h include..._" could address another IWYU [suggestion](https://api.cirrus-ci.com/v1/task/6080090677706752/logs/ci.log):
```diff
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -16,6 +16,7 @@
#include <tinyformat.h>
#include <txdb.h>
#include <uint256.h>
+#include "util/fs.h"
#include <util/time.h>
#include <util/translation.h>
#include <validation.h>
```
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476785071)
@martinus
> Btw. I really links to the changes in your PR updates. Do you use any script to help with that?
No, manual copy-paste :)
FWIW, I borrowed that idea from @ryanofsky.
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476785071)
@martinus
> Btw. I really links to the changes in your PR updates. Do you use any script to help with that?
No, manual copy-paste :)
FWIW, I borrowed that idea from @ryanofsky.
💬 1440000bytes commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476790070)
> I think it's quite unfortunate there is no interest in this feature. How can we expect the Bitcoin Core wallet to behave reasonably if it can't predict the fees well?
There is enough interest if you look at all the comments in this PR and the one opened by @jonasschnelli in 2019 except 3 developers. Could have been useful.
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476790070)
> I think it's quite unfortunate there is no interest in this feature. How can we expect the Bitcoin Core wallet to behave reasonably if it can't predict the fees well?
There is enough interest if you look at all the comments in this PR and the one opened by @jonasschnelli in 2019 except 3 developers. Could have been useful.
💬 russeree commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1476793337)
https://github.com/NYDIG/bitcoin-task-bounty/blob/main/bounties/add-bpf-tracepoints-for-the-mempool.md
This might be of interest. For reference I am no longer and have not been working on my mempool tracepoints PR and I closed it months ago.
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1476793337)
https://github.com/NYDIG/bitcoin-task-bounty/blob/main/bounties/add-bpf-tracepoints-for-the-mempool.md
This might be of interest. For reference I am no longer and have not been working on my mempool tracepoints PR and I closed it months ago.
💬 furszy commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142580308)
> 1. I think this should respect `fRejectLongChains` flag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.
It's being respected:
If the user asked to ignore long-chain constraint (`fRejectLongChains=false`) the last filter include the coins, which means that they aren't discarded, so the `discarded_groups` vector is empty, so `total_unconf_long_chain` is zero, so the error is never hit.
> 2. If `fRejectLongChains` flag is set (the default) then
...
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142580308)
> 1. I think this should respect `fRejectLongChains` flag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.
It's being respected:
If the user asked to ignore long-chain constraint (`fRejectLongChains=false`) the last filter include the coins, which means that they aren't discarded, so the `discarded_groups` vector is empty, so `total_unconf_long_chain` is zero, so the error is never hit.
> 2. If `fRejectLongChains` flag is set (the default) then
...
💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476822455)
Building bdb with DEBUG=1 did not make the `LeakSanitizer` output more usable.
I also tried compiling with gcc (12.2.0) and then running under valgrind (3.18.1). This yielded much more useful info:
```
023-03-20T19:32:09Z [test_legacy] Releasing wallet
2023-03-20T19:32:09Z Shutdown: done
==1744646==
==1744646== HEAP SUMMARY:
==1744646== in use at exit: 2,377 bytes in 12 blocks
==1744646== total heap usage: 296,219 allocs, 296,207 frees, 103,153,206 bytes allocated
==1744646==
...
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476822455)
Building bdb with DEBUG=1 did not make the `LeakSanitizer` output more usable.
I also tried compiling with gcc (12.2.0) and then running under valgrind (3.18.1). This yielded much more useful info:
```
023-03-20T19:32:09Z [test_legacy] Releasing wallet
2023-03-20T19:32:09Z Shutdown: done
==1744646==
==1744646== HEAP SUMMARY:
==1744646== in use at exit: 2,377 bytes in 12 blocks
==1744646== total heap usage: 296,219 allocs, 296,207 frees, 103,153,206 bytes allocated
==1744646==
...
💬 prusnak commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476829212)
> There is enough interest if you look
What I meant to say was "there is no interest in **merging** this feature", updated the comment.
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476829212)
> There is enough interest if you look
What I meant to say was "there is no interest in **merging** this feature", updated the comment.
💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476836735)
> This leak may be fixed in bdb5.3, but I haven't re-checked this.
I tried with `apt install libdb++-dev` version 5.3.21 (?_ and `./configure --with-incompatible-bdb …`
This shows "Using BerkeleyDB version Berkeley DB 5.3.28: (September 9, 2013)" in the log so that's a bit odd.
But in any case there's no leak. Not with valgrind (gcc) and not with the address sanitizer (clang).
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476836735)
> This leak may be fixed in bdb5.3, but I haven't re-checked this.
I tried with `apt install libdb++-dev` version 5.3.21 (?_ and `./configure --with-incompatible-bdb …`
This shows "Using BerkeleyDB version Berkeley DB 5.3.28: (September 9, 2013)" in the log so that's a bit odd.
But in any case there's no leak. Not with valgrind (gcc) and not with the address sanitizer (clang).
💬 LarryRuane commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#issuecomment-1476862325)
Force-pushed again to fix CI failures.
(https://github.com/bitcoin/bitcoin/pull/19690#issuecomment-1476862325)
Force-pushed again to fix CI failures.
💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142628251)
Yes you're right, I somehow managed to confuse myself about the filters. Thanks for patient explanation.
Still something doesn't sit right. So if a coin is discarded because of `max_ancestors` we give this error message, but if it's discarded by the filter `max_ancestors/2` we just say insufficient funds when the coins are there they just don't meet the filters. There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, we don't allow an
...
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142628251)
Yes you're right, I somehow managed to confuse myself about the filters. Thanks for patient explanation.
Still something doesn't sit right. So if a coin is discarded because of `max_ancestors` we give this error message, but if it's discarded by the filter `max_ancestors/2` we just say insufficient funds when the coins are there they just don't meet the filters. There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, we don't allow an
...
👍 TheCharlatan approved a pull request: "Introduce `MockableDatabase` for wallet unit tests"
(https://github.com/bitcoin/bitcoin/pull/26715)
Code review ACK [a9c58a9](https://github.com/bitcoin/bitcoin/pull/26715/commits/a9c58a93bc4929724ff539bb5be457e8b2eb7dc0)
(https://github.com/bitcoin/bitcoin/pull/26715)
Code review ACK [a9c58a9](https://github.com/bitcoin/bitcoin/pull/26715/commits/a9c58a93bc4929724ff539bb5be457e8b2eb7dc0)
💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1142666804)
nit: I don't think this include is required.
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1142666804)
nit: I don't think this include is required.
📝 achow101 opened a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286)
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in `mapWallet`, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is es
...
(https://github.com/bitcoin/bitcoin/pull/27286)
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in `mapWallet`, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is es
...
💬 furszy commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142716211)
> Still something doesn't sit right. So if a coin is discarded because of max_ancestors we give this error message, but if it's discarded by the filter max_ancestors/2 we just say insufficient funds when the coins are there they just don't meet the filters
no, those coins will be added to the last non-customizable filter which has `max_ancestors-1` and `max_descendants-1`.
> There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, w
...
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1142716211)
> Still something doesn't sit right. So if a coin is discarded because of max_ancestors we give this error message, but if it's discarded by the filter max_ancestors/2 we just say insufficient funds when the coins are there they just don't meet the filters
no, those coins will be added to the last non-customizable filter which has `max_ancestors-1` and `max_descendants-1`.
> There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, w
...
💬 ryanofsky commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477004863)
This is puzzling because https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476822455 seems to suggest that memory is leaked while creating the BDB environment, but the commit which triggers the leak 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 does not seem to be changing the way the environment is created or how it is used. The commit does switch from using a function called `CreateWalletDatabase` to another one called `MakeWalletDatabase`, but in both cases the functions are calling th
...
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477004863)
This is puzzling because https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476822455 seems to suggest that memory is leaked while creating the BDB environment, but the commit which triggers the leak 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 does not seem to be changing the way the environment is created or how it is used. The commit does switch from using a function called `CreateWalletDatabase` to another one called `MakeWalletDatabase`, but in both cases the functions are calling th
...
💬 furszy commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477006955)
> What scenario are you envisioning where we have a selection_target of 0, a fee rate of 0, and no pre-selected inputs that we would then want to call SelectCoins? With how it currently works, this would lead to SelectCoins assuming we DID have pre-selected and trying to continue, which is definitely wrong: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L604-L609
That is the "little change" that needs to be done that I mentioned above.
The simplest scenario is what is
...
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477006955)
> What scenario are you envisioning where we have a selection_target of 0, a fee rate of 0, and no pre-selected inputs that we would then want to call SelectCoins? With how it currently works, this would lead to SelectCoins assuming we DID have pre-selected and trying to continue, which is definitely wrong: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L604-L609
That is the "little change" that needs to be done that I mentioned above.
The simplest scenario is what is
...