💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746)
Updated ebfc2a2dddedcbf384f435716d30d8418a77505b -> c7af97113459f50cf33882fa18909a6109e914f4 ([pr26749.13](https://github.com/hebasto/bitcoin/commits/pr26749.13) -> [pr26749.14](https://github.com/hebasto/bitcoin/commits/pr26749.14), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.13..pr26749.14)).

Addressed @MarcoFalke's:
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143242170)
I can see no reasons to keep i. [Removed](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143243113)
> I think an alternative fix would be to simply call `clear()` before `resize()`, and maybe even `reserve()`/construct with the max capacity on construction. This should reduce `malloc` to a single call at `vChecks` construction. Also, this should make `clang-tidy` happy, because of `clear()`. Finally, `clear()` should not change the capacity.

Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477691222)
Might be good to keep the clang-tidy related change in a separate commit, because it is independent (it is independent, because it can be applied on current master unreleated to the changes here in this pull)?

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1477694019)
The linter started [complaining](https://cirrus-ci.com/task/5633626411368448?logs=lint#L224) about "E275 missing whitespace after keyword". Fixed this up in the individual commits.

ac438b10918ef05df2e564734d879699c8aa18f9 -> 6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae ([2022-05-connection-tracepoints.pr.1](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.1) -> [2022-05-connection-tracepoints.pr.2](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.2)
...

💬 fanquake commented on issue "test: `wallet_importdescriptors.py --descriptors` failure":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477699527)
See also https://gist.github.com/fanquake/a458badc73abb47f8c06f009d15e1916 (combined log).

💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477705117)
Bitcoin Core has had fee estimation logic for a long time (`estimatefee` RPC was added in 2014, replaced with `estimatesmartfee` in 2015).

The Bitcoin Core wallet automatically uses this internal feerate estimation logic to decide on the fees of transactions. You need to override things, or use more low-level RPCs, to not use it.

That feerate estimation logic works by looking at how quickly mempool transactions get confirmed, over longer time windows, not by looking at the current mempool comp
...

📝 fanquake locked a pull request: "Year update"
(https://github.com/bitcoin/bitcoin/pull/27240)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477712121)
> Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.

ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).

Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend we merge this chang
...

📝 fanquake locked a pull request: "Updated copyright years"
(https://github.com/bitcoin/bitcoin/pull/27267)
Not sure if this is a thing that should be done, but I noticed that alot of the copyright years are set in the past for the comments.

EDIT:

I did not find any mention about copyright dates and how these should be handled in the https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md file.

📝 fanquake locked a pull request: "build: ignore common editor files"
(https://github.com/bitcoin/bitcoin/pull/27275)
Add IntelliJ and Visual Studio Code editor files to .gitignore. Small QOL change :)

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1143274477)
thinking about this a bit more, I think we should have a more robust way of checking this. iirc, taproot would be `tb1p` on testnet, so this would not work for segwit v1 addresses.

Maybe instead you could do something like `version, payload = bech32_to_bytes` and then check the version. If the version is None, then move on to `payload, version = base58_to_byte` etc.

💬 furszy commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477731516)
> > Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.
>
> ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).
>
> Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend we merge
...

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143232308)
nit: it's actually an argument, but I would just leave it out

```suggestion
* if the key is not found or if uri_parsed is nullptr (most probably from a failed parsing).
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143234062)
nit
```suggestion
* @param[in] uri_parsed is the uri parsed with libevent's evhttp_uri_parse
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143273441)
I think the tests are still more complex than necessary. How about this:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index 0462d3ae3..6ae57bdf0 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -11,42 +11,46 @@

BOOST_FIXTURE_TEST_SUITE(httpserver_tests, BasicTestingSetup)

-static evhttp_uri* GetURIParsed(const std::string uri){
- return evhttp_uri_parse(uri.c_str(
...

👍 willcl-ark approved a pull request: "refactor: Replace GetTimeMicros by SystemClock"
(https://github.com/bitcoin/bitcoin/pull/27233)
tACK faf3f1242

As I was unfamiliar with this code it was a little counter-intuitive to me at first that we truncate the time to seconds, before re-adding the microseconds if `m_log_time_micros` was set, but sure enough it mirrors the old behaviour.

I also left one tiny nit/comment which doesn't need to be addressed.

💬 willcl-ark commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143268447)
nit: as we only use this in one single place (_logging.cpp_) it almost seems to make more sense just to drop this and directly use `std::chrono::system_clock` there? Although having `SteadyClock` and `SystemClock` as the two options perhaps prefereable.

💬 MarcoFalke commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1143290208)
If this is used in more places in the future, it seems convenient to have `NodeClock`, `SteadyClock`, and `SystemClock` in the same header?

💬 dergoegge commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1143291078)
Thinking a little bit about what data should go into the tracepoints.

`m_addr_name`: would it make more sense to put `addr` in binary form (or both)? I'm not sure they are equivalent all the time.
`nKeyedNetGroup`: is derived deterministically from `addr` using random seeds that are created with every `CConnman`. I think that means that across restarts of a node previous net groups loose meaning (i.e. you can't compare them to netgroups of new connections). Maybe `NetGroupManager::GetGroup(a
...