💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142325608)
In commit "wallet: Replace use of purpose strings with an enum" (ea3cc035ddf8f15fd18407231c3ff9edde8891c1)

s/address/addresses/

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142308217)
In commit "wallet: Add wallet/types.h for simple public enum and struct types" (f00f5dad29d81b9700dd84ec08a6e72f915988a0)

Looks like there might be an extra 3 lines of space instead of 2 (but fine to keep if that's intentional)

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142331004)
In commit "doc: Release note for purpose string restriction" (0d18142f595b43070bd07b608d43c9ed9b37cdcc)

Maybe s/restricted/now restricted/ to be clear this is new behavior not background information.

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1476485625)
> But approach wise I'd like us to preserve the intent of the user that the wallet is intended as `manual` (we should pick a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?

I agree, but I think that's orthogonal to this change and should be done in a followup.

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1142342689)
> I can always revert back to https://github.com/bitcoin/bitcoin/commit/f87c39399823e22c553b797cc66fa4063462a32b

Not sure. I will need to move those functions to blockman anyway for some other work, so dropping the changes will only kick the can down the road.

> Both are branching out a bit too much in my taste for the refactor we seek to achieve here.

I can take a look when time allows. In the meantime other reviewers can chime in.

📝 hebasto opened a pull request: "build: Link `libbitcoinkernel` to `libbitcoin_util`"
(https://github.com/bitcoin/bitcoin/pull/27285)
This PR makes `libbitcoinkernel` reuse object files from `libbitcoin_util` instead of compiling them.

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476501591)
Git bisect points to 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 as the culprit (#19619). I verified that this commit triggers the error whereas its parent 3c815cfe54087fd139169161d2fd175e99840e6a doesn't.

It makes sense that this went unnoticed because `wallet_tests` was already suffering from a different memory leak at the time. cc @ryanofsky

💬 fanquake commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476501652)
> This PR makes libbitcoinkernel reuse object files from libbitcoin_util instead of compiling them.

Why

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142351647)
fwiw: This is the "source", I think the repo you linked to is an outdated fork: https://sourceware.org/git/?p=systemtap.git;a=blob;f=includes/sys/sdt.h;h=4075a5ff1c3f09c3913615e62840595efe003c6d;hb=HEAD#l404

But yes, reading that comment I'd agree that it should work. I've added the following to try it but it doesn't compile. I think that's the reason why I added the `TRACEPOINT0()`.

```patch
diff --git a/src/test/util_trace_tests.cpp b/src/test/util_trace_tests.cpp
index 6e3e846b4..488d
...

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1142353299)
Plan to revisit this once I have the time to experiment

💬 0xB10C commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1476519399)
ACK 4b7aec2951fe4595946cdc804b0dec1921d79d05

Reviewed changes since my last ACK with `git range-diff d0c6da92c...4b7aec2951`:
- This adds the entry time to the `mempool:removed` transactions. The docs and the tests are updated accordingly.
- Code reviewed that the `mempool_monitor.py` demo now doesn't store events forever and let the script run for >10min.
- Ran the `interface_usdt_mempool.py` test

The failing CI job looks unrelated.

💬 hebasto commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476529970)
> > This PR makes libbitcoinkernel reuse object files from libbitcoin_util instead of compiling them.
>
> Why

Isn't `libbitcoin_util` supposed to be a reusable build artifact?

And in master branch `libbitcoinkernel_la_SOURCES` and `libbitcoin_util_a_SOURCES` have almost the same source file lists.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142374618)
After some tweaks around the unit tests (can't [test an assertion failure on`BOOST`](https://stackoverflow.com/questions/270542/testing-for-assert-in-the-boost-test-framework) and the a solution using`BOOST_REQUIRE_THROW(function_w_failing_assert(), boost::execution_exception);`works only on windows), this is done now.

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1476547544)
Same error/cause in another test file:

https://cirrus-ci.com/task/4914078427119616?logs=ci#L3898

```
2023-03-20T15:27:58.304000Z TestFramework (INFO): Stopping nodes
2023-03-20T15:27:58.935000Z TestFramework (INFO): Cleaning up /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230320_151651/wallet_transactiontime_rescan_149 on exit
2023-03-20T15:27:58.935000Z TestFramework (INFO): Tests successful
stderr:
Exception in thread Thread-1:
Traceback (most recent call last):

...

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1476552873)
Updated changes:
- Updated the code with a new workaround to avoid failures on unit tests (`httpserver_tests`).

Thanks @stickies-v for your prompt response and your support on quick workarounds on issues raised.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142370781)
A simple getter is best implemented in the header

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142379157)
Why remove `const`?

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142382188)
I think this would benefit from helper function(s) to avoid duplicating all the parsing/memclearing logic and make the tests easier to read.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142376537)
What's the rationale behind moving all this code? I don't think it's necessary?

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142386023)
These changes are unrelated to the segmentation fault and at the very least need to be in a separate commit

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142378663)
seems unrelated to this PR, and no need to touch this otherwise?