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

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142371332)
```suggestion
const evhttp_uri* GetURIParsed() const;
```

💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1476558883)
> Good catch.
>
> I agree to fail when the user disallowed automatic coin selection (`add_inputs=false`) but not seeing why we should fail when it's enabled. The wallet can craft the tx by selecting any available coin.

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

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

💬 prusnak commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476574236)
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?

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476580792)
I [wrote](https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141986299):

> using `warning` for now seems fine, since we don't actually print the word "warning" in the log.

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.

🚀 achow101 merged a pull request: "mempool: Add mempool tracepoints"
(https://github.com/bitcoin/bitcoin/pull/26531)

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142420455)
Sorry, forgot to remove all that.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142421537)
Was unintentional, going to place it on its original location.