💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1142496680)
> re: [#27224 (comment)](https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1137647274)

This is done now

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142506066)
done.

💬 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
...

👍 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

💬 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...

💬 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?

👍 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>
```

💬 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.

💬 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.

💬 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.

💬 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
...

💬 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==
...

💬 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.

💬 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).

💬 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.

💬 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
...

👍 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)

💬 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.

📝 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
...

💬 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
...