💬 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
...
(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
...
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120948639)
Nit, more efficient to not make a copy
```suggestion
std::vector<COutPoint> outpoints_of_tx({outpoint});
m_requested_outpoints_by_txid.emplace(outpoint.hash, std::move(outpoints_of_tx));
```
or (I didn't test this, but I'm pretty sure that since the vector argument is an rvalue, the compiler will do a move)
```suggestion
m_requested_outpoints_by_txid.emplace(outpoint.hash, std::vector<COutPoint>{outpoint});
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120948639)
Nit, more efficient to not make a copy
```suggestion
std::vector<COutPoint> outpoints_of_tx({outpoint});
m_requested_outpoints_by_txid.emplace(outpoint.hash, std::move(outpoints_of_tx));
```
or (I didn't test this, but I'm pretty sure that since the vector argument is an rvalue, the compiler will do a move)
```suggestion
m_requested_outpoints_by_txid.emplace(outpoint.hash, std::vector<COutPoint>{outpoint});
```
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120964573)
Nit, simpler. I don't know if the comment is necessary (the reader is expected to know this about maps), but it may help to document that we're taking advantage of that behavior.
The developer notes do [say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) not to use the `std::map []` syntax, but only for reading.
```suggestion
// This creates the map entry if it doesn't already exist.
m_requested_outpoints_by_txid[outpoint.ha
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120964573)
Nit, simpler. I don't know if the comment is necessary (the reader is expected to know this about maps), but it may help to document that we're taking advantage of that behavior.
The developer notes do [say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) not to use the `std::map []` syntax, but only for reading.
```suggestion
// This creates the map entry if it doesn't already exist.
m_requested_outpoints_by_txid[outpoint.ha
...
💬 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.
(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
...
(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())) {
```
(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};
```
(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
...
(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};
```
(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.
(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.
(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
(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`.
(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.
(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.
(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.
(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
...
(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.
(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.
(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?
(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?