💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142449820)
ok, let me add some helper(s) there.

👍 sipa approved a pull request: "Add pool based memory resource"
(https://github.com/bitcoin/bitcoin/pull/25325)
ACK d87cb99bb37637e26a9e00b9f7de4bc6f44cb79d

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142466765)
Yeah, putting it back, before the compiler was complaining as in the previous approach we were modifying m_uri_parsed in this function.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142467803)
Similar reason to the above comment on`GetURIParsed()` function, back again.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142472126)
I'll take it back.

👍 Sjors approved a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
tACK 2c3a90f663a61ee147d785167a2902494d81d34d

Prefer if you leave it like this and leave log level tweaks to a followup.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142467276)
Funny enough this message comes _after_ "Saw new header". So from the point of view of making the log a bit more intuitive it does make sense to handle logging in net_processing. That might also solve the IBD "problem", since we could log only headers that we didn't request ourselves.

All that said: from a review point of view I prefer punting this to a followup.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142453679)
This creates a nice flood of messages when running with `-debug=validation` right after our headers presync succeeds. But that's probably fine if someone is actively debugging IBD.

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

Instead of extending the PR to modify GUI code, I've kept changes here internal to the wallet and added a note in `setAddressReceiveRequest` about the wallet/gui interface and say how it could be improved in the future.

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