✅ maflcko closed an issue: "test: failure in feature_coinstatsindex.py "
(https://github.com/bitcoin/bitcoin/issues/29746)

💬 maflcko commented on issue "test: failure in feature_coinstatsindex.py ":
(https://github.com/bitcoin/bitcoin/issues/29746#issuecomment-2037230147)
Closing for now, but this can be reopened if it happens again.

💬 ryanofsky commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2037233316)
Merged this since it is a small straightfoward fix, only affects indexing, has 2 acks (ryanofsky, fjahr) and positive comments from other reviewers (mzumsande, sjors, furszy)

💬 fanquake commented on pull request "test: Fix debug recommendation in argsman_tests":
(https://github.com/bitcoin/bitcoin/pull/29805#issuecomment-2037245111)
cc @ryanofsky

💬 sipa commented on pull request "refactor: Remove gmtime*":
(https://github.com/bitcoin/bitcoin/pull/29081#issuecomment-2037251004)
utACK fa9f36babaceba6ab2f88e64bc4bc2956f58871f

💬 mzumsande commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1551733252)
This isn't changed behavior, but with the threshold reduced to 10 minutes it may be a bit more relevant now:
As far as I understand it, for GUI users `ThreadSafeMessageBox()` blocks the calling thread (here `msghand`) until the user has clicked away the warning:
That means that for as long as the user didn't click anything (e.g. because they are afk), most activity stops: No more received messages are processed, the tip is no longer updated etc. The current peers will disconnect us soon (beca
...

💬 Willtech commented on issue "Signmessage doesn't work with segwit addresses":
(https://github.com/bitcoin/bitcoin/issues/10542#issuecomment-2037289362)
Also, [signmessage works in V0.18.1](https://twitter.com/CA_22562_AN/status/1775884518995747193)

https://bitcoincore.org/bin/bitcoin-core-0.18.1/

Professor. Damian A. James Williamson
Wills

⚠️ maflcko opened an issue: "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError"
(https://github.com/bitcoin/bitcoin/issues/29806)
https://cirrus-ci.com/task/5524545770094592?logs=ci#L3290

💬 Eunovo commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2037477967)
Tested ACK https://github.com/bitcoin/bitcoin/pull/29523/commits/3a64b3a339fe55c2642369b00a382231871c283

💬 pablomartin4btc commented on pull request "Don't permit port in proxy IP option":
(https://github.com/bitcoin-core/gui/pull/813#issuecomment-2037524282)
> I'm not convinced that a large regex is "simpler" than checking if a ":" is in the string?

The large one was for the port number, the one I referred as "simpler" was for the IP address: `QRegularExpression ipRegex("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");`. What I meant is that we know in advance that the ":" would be invalid so we could restrict the user to even enter it. As current we could see this (not only ":"):

![image](https://github.com/bitcoin-core/gui/assets/110166421/139224e2-aa0d-44
...

💬 fanquake commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2037552298)
One thing to investigate is if `-ggdb(n)` is useful here at all.

💬 glozow commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237)
nit: why change to int32?

💬 glozow commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552029703)
nit: Could probably just delete the full test case? This isn't a `build_diagram_test` any more, and comparison of oversized chunks is already tested above.

🤔 glozow reviewed a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757#pullrequestreview-1980465571)
code review ACK 43e2f6d160b75e88b39777428c2c1892b962f394

💬 krisisnotok commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2037674193)
? How, like how? My bitcoins are gone?

On Thu, 4 Apr 2024 at 9:59 PM, Gloria Zhao ***@***.***> wrote:

> ***@***.**** commented on this pull request.
>
> code review ACK 43e2f6d
> <https://github.com/bitcoin/bitcoin/commit/43e2f6d160b75e88b39777428c2c1892b962f394>
> ------------------------------
>
> In src/test/fuzz/rbf.cpp
> <https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237>:
>
> > CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
> - int
...

💬 davidgumberg commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2037768188)
reACK https://github.com/bitcoin/bitcoin/commit/2842e51a246b162a586941184b7694f187d7aee7

I think the 30 second dns seed fallback condition here is more likely than I assumed because of the low quality of P2P gossip addresses mentioned by mzumsande above. I experienced it on my first test of this branch on signet and decided to investigate:

In my testing, 6 out of 12 attempts to bootstrap a signet node with a `-seednode` fell back to DNS seeds, and 7 out of 12 attempts to bootstrap a mainne
...

🤔 murchandamus reviewed a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1980784495)
ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4 with nits.

💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552208469)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure" (271599af6d84c871154dc16c7abcadbe5ac08163):

If these node parameters generally are needed for `fill_mempool` to work, would it perhaps make sense to move them to the `fill_mempool` function?

Alternatively, the comment should perhaps be "Lower mempool limit to make it easier to `fill_mempool`"

💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552198747)
In "Move fill_mempool to util function" (cc770e4b36e6f04cbd588ad3e7a19f122c21d38d):

Nit: If a commit is described as a move, I would expect no changes in the code beyond what’s necessary to update the callsites. While these changes are minor and obviously do not entail a functional change, I would have expected the updated commentary and refactor to be a separate commit—I’m wondering whether my understanding of the modus operandi is correct.

💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552232875)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…

…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):

Pet-peeve nit: I suspect that the issues here is rather that the mempool min _feerate_ is not met?

```suggestion
assert "mempool min feerate not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
```