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

🚀 achow101 merged a pull request: "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`"
(https://github.com/bitcoin/bitcoin/pull/26899)

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

🚀 achow101 merged a pull request: "guix: use osslsigncode 2.5"
(https://github.com/bitcoin/bitcoin/pull/27179)

💬 achow101 commented on pull request "rpc: In `utxoupdatepsbt` also look for the tx in the txindex":
(https://github.com/bitcoin/bitcoin/pull/25939#issuecomment-1476594222)
ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98

💬 TheCharlatan commented on pull request "ci: Cache more stuff in the ci images: msan, iwyu, pip, sdks":
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1476594464)
Light code review ACK 5555a336019848d2e19e4533b3d2f34e438e6367

Confirmed that it indeed caches these additional packages. That said, I am not familiar with the build flow through the various scripts and cannot really comment if the code was moved to the correct place, or if conventions were followed.

> Would be good to make progress here.

Yes, I need to get a better scheduling workflow for PR's requiring interaction.

💬 fanquake commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476598738)
NACK. This is pretty literally the opposite of what we want to be doing with the kernel.

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

The while point of having these files enumerated, is so that we know what is currently in the kernel, and can continue to (easily) prune deps, while refactoring. Hiding this away into `libbitcoin_util` only makes this harder/impossible to do, and you're just removing the current, exp
...