💬 paten12 commented on issue "failure in wallet_basic.py --descriptors":
(https://github.com/bitcoin/bitcoin/issues/27249#issuecomment-1496777632)
> https://cirrus-ci.com/task/6282049166770176?logs=ci#L3112
>
> ```shell
> node0 2023-03-12T18:59:07.047547Z [scheduler] [wallet/wallet.h:840] [WalletLogPrintf] [default_wallet] AddToWallet 0a94aab7b4dc8ba22b88ccd6b2c9898936c7d36bf8b3e1789f6083b85f6d752a new
> test 2023-03-12T18:59:07.053000Z TestFramework (ERROR): Assertion failed
> Traceback (most recent call last):
> File "/tmp/cirrus-ci-build/ci/scratch/buil
...

💬 ajtowns commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1157916340)
(Would support changing the default to 0sat/vb though)

💬 martinus commented on pull request "util: implement `noexcept` move assignment & move ctor for `prevector`":
(https://github.com/bitcoin/bitcoin/pull/27334#issuecomment-1496950282)
> The only exception is a slowdown at https://github.com/bitcoin/bitcoin/commit/81f67977f543faca2dcc35846f73e2917375ae79 for the DirectNontrivial benchmark, but all the other improvements look quite significant.

I think the reason why DirectNontrivial got slower when adding `noexcept` is that now the supposedly faster move operations are used when resizing the vector, but since this was implemented as a `swap`, the implementation had to swap the union which I think involves 3 copies behind th
...

💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158081088)
ah, yes. You're right, now I see why SRD will always find a solution if it exists. LGTM

💬 hebasto commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1497000975)
> I am not able to understand exactly what this issue is trying to resolve..I have realised that the python script talked about here generates man-pages for each binary in the src/doc/man directory..but I am not able to understand what is meant by the documentation of arguments of build parameters...can anyone please explain me and clear my confusion regarding the same.

For instance, if your system prevents `./configure` from defining `HAVE_SYSTEM`, the `bitcoind -help` output will not contai
...

💬 hebasto commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1497004515)
Concept ACK.

💬 ajtowns commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569)
ACK 552684976b6df34ce563458f73812e6e494e3b0e

Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing `MinBIP9WarningHeight` with `MinBIP9WarningStartTime` (set it to say 2022-07-01 initially), and using it for `WarningBitsConditionChecker::BeginTime()`, and just bumping it at each release might be worthwhile.

💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158084125)
I think this should be `change_output_size` not `change_spend_size`

💬 hebasto commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412)
Add a copyright header?

💬 Satoshingmx commented on issue "Bitcoin.org/https://www.bitcoin.org/www.bitcoin.org":
(https://github.com/bitcoin/bitcoin/issues/27416#issuecomment-1497096744)
Yes what is your question

💬 josibake commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1497114881)
Concept ACK

> We'd also need historical snapshots of a mempool to do that. If someone has got some and is willing to test, i'd be very interested in seeing the results.

I have historical snapshots and would be happy to help run the test. Although, since running the test is not trivial, I wouldn't make a blocker for the PR.

💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1158209598)
Doesn't the default `memory_order_seq_cst` memory order for `std::atomic<bool> m_request_stop` guarantee the correct behaviour?

:lock: fanquake locked an issue: "Bitcoin.org/https://www.bitcoin.org/www.bitcoin.org"
(https://github.com/bitcoin/bitcoin/issues/27416)

💬 MarcoFalke commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#issuecomment-1497144933)
lgtm ACK 73f4eb511cf80cf52b78627347727ca02774b731

💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497151925)
Tested that 0076bed45eb2b42111fa3f4c95181393c685a42e works.

During review I noticed `fLogIPs` which is controlled by `-logips` which is off by default. This settings seems to be ignored in a bunch of places. I don't know what it's original purpose is. If the issue was peer privacy (which #3764 implies), then even though an ASN is also not an IP, we should probably still hide it when `-logips` is false.

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155)
Right, incoming Tor need special handling. That is already what `CNode::ConnectedThroughNetwork()` does. So it would look like:

```cpp
// For privacy reasons, don't advertise our privacy-network address
// to other networks and don't advertise our other-network address
// to privacy networks.
const Network our_net{entry.first.GetNetwork()};
const Network peers_net{node.ConnectedThroughNetwork()};
if (our_net != peer
...

💬 ismaelsadeeq commented on pull request "test: add coverage for `-bantime`":
(https://github.com/bitcoin/bitcoin/pull/26604#discussion_r1137878941)
tAck but how about adding another coverage to test the ban remaining time after the bantime elapsed.

`time.sleep(secs)`
`banned = self.nodes[1].listbanned()[0]`
`assert_equal(banned["time_remaining"],0) `

💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497207639)
> Unfortunately, I've noticed a regression introduced in this PR.

It works for me on latest master locally and on my CI cluster. Please provide exact steps to reproduce, ideally starting from a fresh install of your Linux.

💬 MarcoFalke commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497232880)
Also https://github.com/bitcoin/bitcoin/pull/25466#discussion_r930891939

📝 ismaelsadeeq opened a pull request: "test: add coverage to rpc_scantxoutset.py "
(https://github.com/bitcoin/bitcoin/pull/27422)

Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.

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