💬 fanquake commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1871225764)
The main outcome I can see from this change is confusion from (maybe less-technical) users, when they see this unusual domain / `maybe-malware` printed to their console/logs:
```bash
2023-12-28T11:54:27.034530Z [dnsseed] Loading addresses from DNS seed dnsseed.bitcoin.dashjr-list-of-p2p-nodes-maybe-malware.us.
```

I don't think we should ship software that will do that.

💬 petertodd commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1871266126)
I agree with @fanquake, so NACK the choice of domain name.

Anyone who actually sees the domain name used and uses it for some purpose will very likely be able to understand that these IP addresses may be anything, so there is no need for this disclaimer. Meanwhile, putting malware in the name just invites confusion and unnecessary support requests from curious people who don't understand what they are seeing.

💬 Sjors commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1871307676)
I believe I've processed all feedback on this PR into my PR, either as documentation, TODO or actually implemented.

💬 kristapsk commented on pull request "getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1871335533)
Concept ACK

💬 luke-jr commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-1871354004)
>Not sure. Wasn't the goal the exact opposite, so that it is easier to cat the hashes once and as many signatures as one wants?

You can still do that with the content in the file. Furthermore, this change does not _require_ us to produce an attached SHA256SUMS.asc, it only _enables_ us to do so if desired.

💬 luke-jr commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#discussion_r1437791949)
Done

💬 luke-jr commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1871362078)
Seems like if debug log is the concern, we could just mask it there... Doesn't seem like a good reason to make it easier for scammers by removing the warning?

💬 1440000bytes commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1871364149)
NACK with this domain and I shared it in the first line of my last comment. I agree with _fanquake_ although domain could even get suspended for such domain name and some of the IPs it gets resolved to.

This is important even if not directly related to PR but domain and author: https://github.com/bitcoin/bitcoin/pull/29145#pullrequestreview-1797445282

With these warnings on **luke.dashjr.org** I am not okay with author running a DNS seed for bitcoin core:

> WARNING: In November and Dece
...

🤔 murchandamus reviewed a pull request: "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target"
(https://github.com/bitcoin/bitcoin/pull/29076#pullrequestreview-1798320554)
ACK e03d6f7ed534f423f58236866f8e83beee1871e1

🤔 murchandamus reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-1798367193)
ACK 0c0022311a0cffc4c03b3763c9b60f93ede13687

While I am pretty sure that the reorg logic should handle any transactions getting added or removed from the wallet, it might be useful to add a test that the `m_txos` set would be correct after a reorg, if that’s not obvious.

💬 murchandamus commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1437849004)
I have the impression that `m_txos` is also used to look up information on old transactions and we therefore want to keep all transaction outputs the wallet ever had.

Would it perhaps make sense to also have a reduced set that only keeps the potentially _unspent_ TXOs? I could see a wallet that participates in a few dozen transactions every day have magnitudes more TXOs than UTXOs. So perhaps the work the wallet has to perform every time it builds a transaction could be shortcut by storing e.
...

💬 murchandamus commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1437851827)
Ah, nevermind, I just saw https://github.com/bitcoin/bitcoin/pull/27865

🤔 murchandamus reviewed a pull request: "wallet: Track no-longer-spendable TXOs separately"
(https://github.com/bitcoin/bitcoin/pull/27865#pullrequestreview-1798375168)
Concept ACK

💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#issuecomment-1871465548)
ACK 91dc48c14825a9075a57c1eefda202b83b6346ba

📝 1440000bytes opened a pull request: "Remove `dnsseed.bitcoin.dashjr.org` temporarily"
(https://github.com/bitcoin/bitcoin/pull/29149)
Rationale:

- Only this seeder is giving different results that include older versions: https://github.com/bitcoin/bitcoin/pull/29145#pullrequestreview-1797445282

- Warnings on **luke.dashjr.org** reduces the confidence to use this as DNS seed in bitcoin core: https://pastebin.com/raw/Cwk2a1xr

- He is not following [DNS seed policy](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) rule 0 and 1

🤔 murchandamus reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1777863994)
Concept ACK, did a quick skim review, will review more thoroughly

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1437884761)
Shouldn’t this be greater or equal to _exceed_ the original?

> 6. The package feerate (total package fee / total package vsize) must exceed the min(individual feerate, ancestor feerate) of every transaction that would be evicted (direct and indirect conflicts).

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424218735)
Nit: Did you mean _replacEable_?

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1437895090)
Done

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1437895284)
I've added a test that does this.