🤔 Sjors reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1714571006)
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I'm not very familiar with mempool code.
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1714571006)
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I'm not very familiar with mempool code.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382967058)
ee79f05f1fef57bfbea24333a46e525385d02319: `{`
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382967058)
ee79f05f1fef57bfbea24333a46e525385d02319: `{`
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller `checkChainLimits` of is the wallet's `CreateTransactionInternal`, which will say "Transaction has too long of a mempool chain" regardless of which limit is hit.
A (wallet) followup should propagate `err_string` up.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller `checkChainLimits` of is the wallet's `CreateTransactionInternal`, which will say "Transaction has too long of a mempool chain" regardless of which limit is hit.
A (wallet) followup should propagate `err_string` up.
💬 maflcko commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794414689)
Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794414689)
Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383019317)
ee79f05f1fef57bfbea24333a46e525385d02319: this fixes the dangling warning:
```cpp
const CTxMemPoolEntry& entry{Assert(m_pool.GetEntry(txid)).value()};
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383019317)
ee79f05f1fef57bfbea24333a46e525385d02319: this fixes the dangling warning:
```cpp
const CTxMemPoolEntry& entry{Assert(m_pool.GetEntry(txid)).value()};
```
💬 Sjors commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794431063)
I was still waiting for feedback from maintainers, but I'll just drop the last commit.
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794431063)
I was still waiting for feedback from maintainers, but I'll just drop the last commit.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383022517)
Thank, might be a good idea to bracket initialize in other places too.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383022517)
Thank, might be a good idea to bracket initialize in other places too.
💬 willcl-ark commented on pull request "doc: Add offline signing tutorial":
(https://github.com/bitcoin/bitcoin/pull/28363#issuecomment-1794436797)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Looks good to me!
(https://github.com/bitcoin/bitcoin/pull/28363#issuecomment-1794436797)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Looks good to me!
💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1794443638)
Here's a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).
Is there a reason not to cover more of `DescriptorScriptPubKeyMan` in this PR? e.g. `DescriptorScriptPubKeyMan::SignTransaction` or `DescriptorScriptPubKeyMan::FillPSBT`.
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1794443638)
Here's a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).
Is there a reason not to cover more of `DescriptorScriptPubKeyMan` in this PR? e.g. `DescriptorScriptPubKeyMan::SignTransaction` or `DescriptorScriptPubKeyMan::FillPSBT`.
🚀 fanquake merged a pull request: "build: Drop no longer needed MSVC warning suppressions"
(https://github.com/bitcoin/bitcoin/pull/28798)
(https://github.com/bitcoin/bitcoin/pull/28798)
💬 fanquake commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794459605)
```bash
test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794459605)
```bash
test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
👍 dergoegge approved a pull request: "fuzz: Avoid utxo_total_supply timeout (take 2)"
(https://github.com/bitcoin/bitcoin/pull/28789#pullrequestreview-1714698328)
ACK fa7ba926300f44b350d04262e569cc607e4991d8
(https://github.com/bitcoin/bitcoin/pull/28789#pullrequestreview-1714698328)
ACK fa7ba926300f44b350d04262e569cc607e4991d8
💬 maflcko commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794494686)
> I was still waiting for feedback from maintainers
DrahtBot has a summary comment, which has links to all feedback, including that of maintainers, see https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1750139214
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794494686)
> I was still waiting for feedback from maintainers
DrahtBot has a summary comment, which has links to all feedback, including that of maintainers, see https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1750139214
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794513714)
Updated a20c1d2b81a702f97267d4f098f13ed80e9320cf -> 836a0a7566e166556f80303a9b3dd9560587159c ([simplifyMemPoolInteractions_7](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_7) -> [simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_7..simplifyMemPoolInteractions_8))
* Addressed @Sjors's [comment](https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794513714)
Updated a20c1d2b81a702f97267d4f098f13ed80e9320cf -> 836a0a7566e166556f80303a9b3dd9560587159c ([simplifyMemPoolInteractions_7](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_7) -> [simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_7..simplifyMemPoolInteractions_8))
* Addressed @Sjors's [comment](https://github.com/bitcoin/bit
...
🤔 ismaelsadeeq reviewed a pull request: "MiniMiner changes for package linearization"
(https://github.com/bitcoin/bitcoin/pull/28762#pullrequestreview-1714693345)
Strong ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9
The whole mempool transactions can also be used to create `manual_entries` and `descendant_caches` and use the new constructor to build a block template to get a fee rate histogram of the block template, with just a modification to `BuildMockTemplate` to enable getting the block template fee rate histogram.
it will be more desirable to modify this than `BlockAssembler`.
Due to the use of `GatherClusters`, which has a 500 transaction D
...
(https://github.com/bitcoin/bitcoin/pull/28762#pullrequestreview-1714693345)
Strong ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9
The whole mempool transactions can also be used to create `manual_entries` and `descendant_caches` and use the new constructor to build a block template to get a fee rate histogram of the block template, with just a modification to `BuildMockTemplate` to enable getting the block template fee rate histogram.
it will be more desirable to modify this than `BlockAssembler`.
Due to the use of `GatherClusters`, which has a 500 transaction D
...
💬 ismaelsadeeq commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1383041819)
I think just stating that this is a constructor where `MiniMinerMempoolEntry` are constructed manually is enough as this can be used with mempool transactions, but with the intent of building a block template to get a mempool fee rate histogram
```suggestion
* It is assumed that all
```
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1383041819)
I think just stating that this is a constructor where `MiniMinerMempoolEntry` are constructed manually is enough as this can be used with mempool transactions, but with the intent of building a block template to get a mempool fee rate histogram
```suggestion
* It is assumed that all
```
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794571887)
Updated 836a0a7566e166556f80303a9b3dd9560587159c -> 11b8735852bd7439c4a07c88d07d7dfb13a3c7b7 ([simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8) -> [simplifyMemPoolInteractions_9](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_8..simplifyMemPoolInteractions_9))
* Fixed msvc build error by using `auto` instead of `CTxMemPo
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794571887)
Updated 836a0a7566e166556f80303a9b3dd9560587159c -> 11b8735852bd7439c4a07c88d07d7dfb13a3c7b7 ([simplifyMemPoolInteractions_8](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_8) -> [simplifyMemPoolInteractions_9](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_8..simplifyMemPoolInteractions_9))
* Fixed msvc build error by using `auto` instead of `CTxMemPo
...
⚠️ dergoegge opened an issue: "fuzz: Re-introduce i2p target"
(https://github.com/bitcoin/bitcoin/issues/28803)
We deleted the i2p target (#28692) because it wasn't effective at covering the i2p code and as a result never found any bugs. Someone should re-work the target and then re-introduce it to the repo.
Some of the short comings of the previous harness were identified:
* The temporary file created for the private key should be deleted at the end of each iteration (https://github.com/bitcoin/bitcoin/blob/106ab20f121f14d021725c8a657999079dbabfc1/src/test/fuzz/i2p.cpp#L34), or better yet don't use a
...
(https://github.com/bitcoin/bitcoin/issues/28803)
We deleted the i2p target (#28692) because it wasn't effective at covering the i2p code and as a result never found any bugs. Someone should re-work the target and then re-introduce it to the repo.
Some of the short comings of the previous harness were identified:
* The temporary file created for the private key should be deleted at the end of each iteration (https://github.com/bitcoin/bitcoin/blob/106ab20f121f14d021725c8a657999079dbabfc1/src/test/fuzz/i2p.cpp#L34), or better yet don't use a
...
💬 willcl-ark commented on issue "High CPU load when network traffic page left open":
(https://github.com/bitcoin-core/gui/issues/768#issuecomment-1794692567)
Tested again with 26.0rc2 binary from bitcoincore.org, and the issue is still present for me. So long as the graph content is relatively "varied", then one CPU constantly thrashes between ~60-100%.
Seems to happen even more readily if I set the scale to 5m where even with fewer data points shown (I think) it pretty much just pins a single core to 100%
Tested again with 26.0rc2 binary from bitcoincore.org, and the issue is still present for me. So long as the graph content is relatively "varied", then one CPU constantly thrashes between ~60-100%.
Seems to happen even more readily if I set the scale to 5m where even with fewer data points shown (I think) it pretty much just pins a single core to 100%
![image](https://github.com/bitcoin-cor
...
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383242173)
```suggestion
CTxMemPoolEntry* GetEntry(const uint256& hash) const LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(cs);
```
Instead of optional ref, could just use a pointer?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383242173)
```suggestion
CTxMemPoolEntry* GetEntry(const uint256& hash) const LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(cs);
```
Instead of optional ref, could just use a pointer?