💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1448682269)
> Thinking further about this, if the "privacy" option is enabled, the transactions screen has no usage, thus shouldn't be accessible for the user.

> We could either block the view when the privacy option is enabled, or could create an empty screen image to replace the transactions list when privacy is enabled.

Yeah, I realised what you are saying as soon as I fixed the negative amount issue looking at the type transactions, date-time, addresses columns, the red foreground colour due to th
...

💬 ryanofsky commented on pull request "Deduplicate bitcoind and bitcoin-qt init code":
(https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1120648056)
re: https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1120039071

Thanks, applied change

💬 ryanofsky commented on pull request "Deduplicate bitcoind and bitcoin-qt init code":
(https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1120648916)
re: https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1119622390

> nit: s/to whether/whether to/

Thanks, fixed comment

💬 ryanofsky commented on pull request "Deduplicate bitcoind and bitcoin-qt init code":
(https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1120647673)
re: https://github.com/bitcoin/bitcoin/pull/27150#discussion_r1120047778

Thanks, applied change

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1120662435)
What's the purpose of this annotation `Any` here?

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1120688318)
Maybe I'm reading this wrong, but it looks like the `reversed()` on L288 can just be factored in to this first sort? That might make the actual filtering logic cleaner.

💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1448877559)
> I assume you meant immature coinbases, rather than unconfirmed ones. Which tests currently rely on those? Can't really imagine a case where an unspendable UTXO would be of any use (only if mining of new blocks happen between fetching the UTXO and actually spending them, but then we can probably change those tests to fetch the UTXO later?).

One of the examples I found:

In `p2p_segwit.py`:
https://github.com/bitcoin/bitcoin/blob/cb40639bdf04bab31bcdceb3bf941e9bade8317a/test/functional/p2
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1448970703)
Thanks for the reviews!

> Seeing all that effort to pass around the hacky fastprune arg (which doesn't make any sense outside of functional tests) seems a bit sad, so I wonder if it's really inevitable.
> Did you consider whether it's possible to also move BlockFileSeq into blockmanager? I guess it doesn't work just because of one remaining call site for node::ReadBlockFromDisk from zmqpublishnotifier?

Yes, totally echo this sentiment. Indeed BlockFileSeq and its dependants are not class
...

📝 brunoerg converted_to_draft a pull request: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177)
Fixes #27129

To avoid `bad-txns-premature-spend-of-coinbase` error,
`get_utxo` doesn't return by default immature coinbase.

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1449025828)
Disabling the Transaction tab when the user sets the mask value option:


Also, if you are already in the Transaction tab, switch/ change focus to the overview page as shown here:


💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120968022)
```suggestion
// Parent + child with v3 in the mempool. Child is allowed as long as it is under V3_CHILD_MAX_WEIGHT.
```

💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120967545)
```suggestion
* 2. The tx must be within V3_CHILD_MAX_WEIGHT
```

💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120974200)
Seems like this is not used anywhere:
```suggestion
```

💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120982825)
Missing assertions for the replaced-tx checks:
```suggestion
assert submitres2["replaced-transactions"] == [tx.rehash() for tx in package_txns1]
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
submitres3 = node.submitpackage(package_hex3)
assert submitres3["replaced-transactions"] == [tx.rehash() for tx in package_txns2]
```

💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120972484)
```suggestion
coin = self.coins.pop(0)
```

💬 theStack commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1120985992)
```suggestion
// For example, the mempool contains a large, low-feerate transaction A (99,000vB, 1sat/vB feerate).
// Transaction A has a small, high-feerate child B (1,000vB, 101sat/vB). The user wants to
```

or maybe

```suggestion
// For example, the mempool contains a large, low-feerate transaction A (99,000vB, 1sat/vB feerate)
// which has a small, high-feerate child B (1,000vB, 101sat/vB). The user wants to
```

👍 jarolrod approved a pull request: "Update translations for 25.0 soft translation string freeze"
(https://github.com/bitcoin/bitcoin/pull/27169)
ACK 9172cc672ea99eac6d0210e8b793ca030c20e179

Confirming that we are following the steps laid out in [release-process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for translations. After replicating the steps I have a zero-diff with this branch.

💬 jarolrod commented on pull request "guix: switch to some `minimal` versions of packages in our manifest":
(https://github.com/bitcoin/bitcoin/pull/27172#issuecomment-1449439152)
GUIX hashes:
```
220003bd9c9cb840444494232b01b3d9e17ddda007abfd1b3a1001662b5f24c6 guix-build-2c9eb4afe1f5/output/aarch64-linux-gnu/SHA256SUMS.part
cc7f6e969a37d66164aad138635ea4ca1bb30eff2ed59a16c6b4af716824e4f1 guix-build-2c9eb4afe1f5/output/aarch64-linux-gnu/bitcoin-2c9eb4afe1f5-aarch64-linux-gnu-debug.tar.gz
6f1afad24db86220a29f8e3ee9170201b5ece045e00ce94d1000f0541a111a4d guix-build-2c9eb4afe1f5/output/aarch64-linux-gnu/bitcoin-2c9eb4afe1f5-aarch64-linux-gnu.tar.gz
776ae93e37ddddb9c3
...

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120543538)
```suggestion
// Copyright (c) 2023 The Bitcoin Core developers
```

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120992192)
Not a bug, but I noticed that the only reference to `m_to_be_replaced` outside the constructor is an `Assume` in `SanityCheck()`, I wonder if it's worth `m_to_be_replaced` being a class member. If we give up the `Assume`, which doesn't seem to be doing that much heavy lifting, then `m_to_be_replaced` could be a local variable in the constructor.

Another nice thing about making `m_to_be_replaced` a local variable is that its entries can be pointers to transactions, instead of entire txid hashe
...