💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121191113)
Maybe this 500 can be a `constexpr` in the `MiniMiner` class? I probably wouldn't suggest this if this is the only place it occurs, but the tests specify this value too. It might be nice to be able to change this limit by changing a single line of code.

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121184614)
Can we remove this code? (I prefer avoiding special cases.) The only effect difference I can see by eliminating this is that, with it, `m_ready_to_calculate` will remain `true`, whereas without it, it will be `false`. Does that matter? I'm unsure of the purpose of `m_ready_to_calculate` (in this PR it's only used by test code, although I understand it may be used in the follow-on PR). Seems like once the constructor runs, it should be ready to calculate, no matter what. There's nothing else that
...

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121220463)
```suggestion
if (!m_to_be_replaced.count(txiter->GetTx().GetHash())) {
```

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121223393)
```suggestion
const bool remove_desc{to_be_replaced.count(&tx_desc) > 0};
```

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121215437)
This isn't a very efficient data structure, because each entry's key, the txid, is repeated in all of its vector's entries (because a `COutPoint` includes the txid). All you need is the index (`COutPoint::n`) So this could be:
```
std::map<uint256, std::vector<uint32_t>> m_requested_outpoints_by_txid;
```
although the name would be wrong, it's no longer "requested outpoints". Elsewhere in the code where you need a `COutPoint` (I think there are only two places), you can construct it from
...

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121222211)
```suggestion
const bool remove{m_to_be_replaced.count(txid) > 0};
```

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1449443895)
Updated 88e88a5 -> f87c39399823e22c553b797cc66fa4063462a32b ([removeBlockstorageArgs_5](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_5) -> [removeBlockstorageArgs_6](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_5..removeBlockstorageArgs_6)) with linter fix.

💬 wtogami commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449474465)
Could we please pick a different name for old witnessless syncing than "pruned"? It's different from the old pruned node type that entirely discards old blocks. Old witnessless nodes could reindex and rescan as well as provide old witnessless blocks to other nodes doing IBD of this type. We need a new name for this.

👍 w0xlt approved a pull request: "doc: Expand scantxoutset help text to cover tr() and miniscript"
(https://github.com/bitcoin/bitcoin/pull/27155)
ACK https://github.com/bitcoin/bitcoin/pull/27155/commits/ca605f015dc8fbabbc6c0640d87429d6bf8f553f

💬 w0xlt commented on pull request "doc: Expand scantxoutset help text to cover tr() and miniscript":
(https://github.com/bitcoin/bitcoin/pull/27155#discussion_r1121339332)
Could also include `rawtr(<pubkey>) P2TR output with the specified key as output key`.

📝 Sjors opened a pull request: "26032 followups"
(https://github.com/bitcoin/bitcoin/pull/27180)
Followups for #26032. So far nothing major.

💬 Sjors commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1121378441)
Added to a followup PR #27180.

💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449640124)
I think it's fine to have a temporary hoop here, as long as it's documented.

💬 hebasto commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449655488)
Well, speaking of the removed questionable [commit](https://github.com/bitcoin/bitcoin/commit/a25daf76b5c6659e9002d6ae08a7907933fc120b), I've finally managed to provide steps to reproduce unexpected behavior _without_ that commit:
1. Complete all tasks.
2. Re-run, for example, the "32-bit + dash [gui] [CentOS 8]" task. As expected, ccache achieves 100% hit.
3. Add a commit which does _not_ change any code and run CI job. It is natural to expect that ccache will 100% hit this time as well. But
...

💬 Sjors commented on pull request "rpc: make importaddress compatible with descriptors wallet":
(https://github.com/bitcoin/bitcoin/pull/27034#discussion_r1121399359)
> upgraded the legacy wallet into a descriptors wallet

I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.

💬 Sjors commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1121419524)
47bc99cddc518dc41741c5dcc17f9e629e162449 This is probably fine, but there's a bunch a places that we have to make sure are never reached. For example in `FindBlockPos(` we do `_blockfile_info[nFile]` where `nFile = ... pos.nFile`. I'll have to review that a bit more.

💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449715715)
It would be good to double check if this is recommended "officially" by Cirrus CI. In any case `CIRRUS_WORKING_DIR` isn't under `/tmp/` on macos, so maybe it makes sense for that reason already?

💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449717685)
I guess instead of `${CIRRUS_WORKING_DIR}/ccache_dir` you can also just write `ccache_dir`, as `CIRRUS_WORKING_DIR` is implicit?

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449721171)
Ok, fair enough. Also, I guess no one is forced to upgrade their script. Anyone is free to use the previous version of the python script as long as they want.

💬 glozow commented on pull request "26032 followups":
(https://github.com/bitcoin/bitcoin/pull/27180#discussion_r1121471409)
trailing whitespace here

💬 dergoegge commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449815580)
@wtogami This PR does not introduce a new witnessless archival mode (which is what you are describing iiuc). It only makes pruned nodes skip downloading the witnesses up to the assume-valid point (since they're not validated and deleted shortly after anyway).

I would agree that witnessless archival nodes should not be called "pruned", but again that is not what this PR is introducing.