💬 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
...
💬 achow101 commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477072935)
I've been able to reproduce the leak that valgrind finds on master. It appears to be related to the `-privdb` option; looking at the BDB source, the problem occurs inside a block of code guarded by a check for the `DB_PRIVATE` flag. Disabling that with `-privdb=0` "resolves" the leak.
The bisect appears to be incorrect as the parent commit shows the same issue for me. I don't think there's really a solution for us other than upgrading BDB or ditching it entirely.
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477072935)
I've been able to reproduce the leak that valgrind finds on master. It appears to be related to the `-privdb` option; looking at the BDB source, the problem occurs inside a block of code guarded by a check for the `DB_PRIVATE` flag. Disabling that with `-privdb=0` "resolves" the leak.
The bisect appears to be incorrect as the parent commit shows the same issue for me. I don't think there's really a solution for us other than upgrading BDB or ditching it entirely.
👍 theStack approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261)
re-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
(https://github.com/bitcoin/bitcoin/pull/26261)
re-ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477135406)
Kicked off a bitcoinperf run; will have some results tomorrow.
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477135406)
Kicked off a bitcoinperf run; will have some results tomorrow.
👍 theStack approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
Code-review ACK 6514352829364ab1ccff3a58faf6ca6ef4aafbbb
(https://github.com/bitcoin/bitcoin/pull/27257)
Code-review ACK 6514352829364ab1ccff3a58faf6ca6ef4aafbbb
💬 theStack commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1142807687)
in commit d816bcced0c1afb22f97df4650a35301a25cd9de: nit: since `fMoreWork` is unused above, could reduce it's scope and declare/init it right here:
```suggestion
bool fMoreWork = poll_result->second;
```
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1142807687)
in commit d816bcced0c1afb22f97df4650a35301a25cd9de: nit: since `fMoreWork` is unused above, could reduce it's scope and declare/init it right here:
```suggestion
bool fMoreWork = poll_result->second;
```
💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-1477159508)
Closing for now. We can revisit this at some later date.
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-1477159508)
Closing for now. We can revisit this at some later date.
✅ instagibbs closed a pull request: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398)
(https://github.com/bitcoin/bitcoin/pull/26398)