💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626346904)
In commit "Introduce Miner interface" (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
Not important, but could consider renaming this class.
- interfaces::Node is an interface used by the gui to control the node
- interfaces::Wallet is an interface used by the gui to control the node
- interface::Chain is an interface used by the wallet and indexes to sync with the chain
interfaces::Miner sounds like an interface that would be used to control a miner, which is not really what this is. Mayb
...
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626346904)
In commit "Introduce Miner interface" (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
Not important, but could consider renaming this class.
- interfaces::Node is an interface used by the gui to control the node
- interfaces::Wallet is an interface used by the gui to control the node
- interface::Chain is an interface used by the wallet and indexes to sync with the chain
interfaces::Miner sounds like an interface that would be used to control a miner, which is not really what this is. Mayb
...
💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626328774)
In commit "rpc: getblocktemplate getTip() via Miner interface" (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
It looks like all the other `Miner` parameters and return values are serializable except this CBlockIndex pointer.
This is fine for this PR, but if you wanted to support stratum code running in another process and calling this, you could change this return value to a struct containing whatever information the miner needs to know about the tip (block hash, previous block hash, block ti
...
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626328774)
In commit "rpc: getblocktemplate getTip() via Miner interface" (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
It looks like all the other `Miner` parameters and return values are serializable except this CBlockIndex pointer.
This is fine for this PR, but if you wanted to support stratum code running in another process and calling this, you could change this return value to a struct containing whatever information the miner needs to know about the tip (block hash, previous block hash, block ti
...
💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626320289)
re: https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1621071328
In commit "Introduce Miner interface" (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
> @ryanofsky I'm not sure what the design philophy is behind `Ensure` and `EnsureAny`. The other methods here return more low level objects.
This looks good to me. Ensure functions are just supposed to make it possible to access node/wallet state from RPCs in a type-safe way. There are similar functions for the wallet in src/wallet/rpc
...
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626320289)
re: https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1621071328
In commit "Introduce Miner interface" (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
> @ryanofsky I'm not sure what the design philophy is behind `Ensure` and `EnsureAny`. The other methods here return more low level objects.
This looks good to me. Ensure functions are just supposed to make it possible to access node/wallet state from RPCs in a type-safe way. There are similar functions for the wallet in src/wallet/rpc
...
💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626379012)
re: https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1621067334
> I guess I should change the `BlockAssembler` constructor
In commit "rpc: call CreateNewBlock via miner interface" (f04cf9f75054573c204245f7bb1e6813cce1fed8)
I don't actually understand the TODO here. The way the mempool is passed seems ok. You could do mempool.get() instead of &*mempool, but both should be equivalent.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626379012)
re: https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1621067334
> I guess I should change the `BlockAssembler` constructor
In commit "rpc: call CreateNewBlock via miner interface" (f04cf9f75054573c204245f7bb1e6813cce1fed8)
I don't actually understand the TODO here. The way the mempool is passed seems ok. You could do mempool.get() instead of &*mempool, but both should be equivalent.
💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626371955)
In commit "rpc: getblocktemplate getTip() via Miner interface" (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
Would suggest introducing a `CBlockIndex* tip{miner.getTip()};` variable, using it where possible, and updating it as needed instead of calling `miner.getTip()` repeatedly in this function.
It seems like the reason it is ok to make multiple getTip() calls is that cs_main is locked for most of this function. But more ideally, this function would not be locking cs_main directly, only cal
...
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626371955)
In commit "rpc: getblocktemplate getTip() via Miner interface" (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
Would suggest introducing a `CBlockIndex* tip{miner.getTip()};` variable, using it where possible, and updating it as needed instead of calling `miner.getTip()` repeatedly in this function.
It seems like the reason it is ok to make multiple getTip() calls is that cs_main is locked for most of this function. But more ideally, this function would not be locking cs_main directly, only cal
...
💬 ryanofsky commented on pull request "Introduce Miner interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626351637)
In commit "rpc: call TestBlockValidity via miner interface" (d880aa0ebe374394b2f8c061ebc7d428f899b59b)
Maybe unintended whitespace change here
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1626351637)
In commit "rpc: call TestBlockValidity via miner interface" (d880aa0ebe374394b2f8c061ebc7d428f899b59b)
Maybe unintended whitespace change here
💬 luke-jr commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2148087913)
You forgot to update doc/dependencies.md
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2148087913)
You forgot to update doc/dependencies.md
💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2148114593)
IMO, the first thing to do would be for someone who's familiar with the format of these block files to look at the corrupted files and see if they can figure out what code may have stomped on the blocks (it might be obvious, like a fragment of p2p networking data in the middle of a block -- you never know until you look).
The most likely scenario is that this is a latent software bug that will show up on any machine if its under memory pressure and heavy paging. In my experience, finding pro
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2148114593)
IMO, the first thing to do would be for someone who's familiar with the format of these block files to look at the corrupted files and see if they can figure out what code may have stomped on the blocks (it might be obvious, like a fragment of p2p networking data in the middle of a block -- you never know until you look).
The most likely scenario is that this is a latent software bug that will show up on any machine if its under memory pressure and heavy paging. In my experience, finding pro
...
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148130643)
@hebasto Good idea, self copy-assignment was indeed broken! Fixed, and added tests for self copy/move/swap.
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148130643)
@hebasto Good idea, self copy-assignment was indeed broken! Fixed, and added tests for self copy/move/swap.
💬 brunoerg commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2148203825)
> Not sure the asmap feature cares if your map is based on BGP or something else?
AFAIK I think it doesn't care, but it's the expected. Which alternatives do you see?
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2148203825)
> Not sure the asmap feature cares if your map is based on BGP or something else?
AFAIK I think it doesn't care, but it's the expected. Which alternatives do you see?
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582)
> @hebasto Good idea, self copy-assignment was indeed broken! Fixed, and added tests for self copy/move/swap.
I suspect this is probably broken in several of our other classes as well. Speaking for myself at least, I almost always forget about correctness there and never look for it in review.
FWIW I tried using the [bugprone-unhandled-self-assignment](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-self-assignment.html) clang-tidy check which does catch the problem, but
...
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582)
> @hebasto Good idea, self copy-assignment was indeed broken! Fixed, and added tests for self copy/move/swap.
I suspect this is probably broken in several of our other classes as well. Speaking for myself at least, I almost always forget about correctness there and never look for it in review.
FWIW I tried using the [bugprone-unhandled-self-assignment](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-self-assignment.html) clang-tidy check which does catch the problem, but
...
💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626494800)
nit:
This `self.nodes[0].blocks_path / "blk..." / ...` is a [`Path`](https://docs.python.org/3/library/pathlib.html) so you don't need to `import os` to call `os.rename` to rename it, you can instead:
```suggestion
(self.nodes[0].blocks_path / "blk00000.dat").rename("blk00000.dat.copy")
```
Most of the things you could do to files/directories with `os` can also be done with `Path`.
(The `.rename` is aware of which bit of the path is the file so you don't need to give it the
...
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626494800)
nit:
This `self.nodes[0].blocks_path / "blk..." / ...` is a [`Path`](https://docs.python.org/3/library/pathlib.html) so you don't need to `import os` to call `os.rename` to rename it, you can instead:
```suggestion
(self.nodes[0].blocks_path / "blk00000.dat").rename("blk00000.dat.copy")
```
Most of the things you could do to files/directories with `os` can also be done with `Path`.
(The `.rename` is aware of which bit of the path is the file so you don't need to give it the
...
💬 AngusP commented on pull request "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure":
(https://github.com/bitcoin/bitcoin/pull/30174#discussion_r1626503734)
There's the "maximum 2-hour block timestamp difference" acceptance rule, but I don't think Valgrind is *that* slow 😆
(https://github.com/bitcoin/bitcoin/pull/30174#discussion_r1626503734)
There's the "maximum 2-hour block timestamp difference" acceptance rule, but I don't think Valgrind is *that* slow 😆
👍 AngusP approved a pull request: "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure"
(https://github.com/bitcoin/bitcoin/pull/30174#pullrequestreview-2097230263)
Untested ACK fa4f0decb9106b5046fda76baf2f7ed44a5d3c62
(https://github.com/bitcoin/bitcoin/pull/30174#pullrequestreview-2097230263)
Untested ACK fa4f0decb9106b5046fda76baf2f7ed44a5d3c62
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148252096)
@theuni What if the comparison is changed to `if (this == &other) return *this;`?
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148252096)
@theuni What if the comparison is changed to `if (this == &other) return *this;`?
🤔 instagibbs reviewed a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2096682721)
reviewed through 9b04e8864b2807c94a03b9093cfa8c836d7c7116, but I need to look closer at the fuzz cases
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2096682721)
reviewed through 9b04e8864b2807c94a03b9093cfa8c836d7c7116, but I need to look closer at the fuzz cases
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626177967)
you can ignore this if you don't like it
```Suggestion
/** m_buffer + m_offset points to first object in queue. m_offset = 0 if m_capacity is 0; otherwise
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626177967)
you can ignore this if you don't like it
```Suggestion
/** m_buffer + m_offset points to first object in queue. m_offset = 0 if m_capacity is 0; otherwise
```
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626321220)
Putting bounds on the capacity growth seems useful here and a few other spots, even if it doesn't necessarily have to be this tight.
```
if (old_cap > old_size) {
assert(real[idx].capacity() == old_cap);
} else {
assert(real[idx].capacity() > old_cap);
assert(real[idx].capacity() <= 2 * (old_cap + 1));
}
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626321220)
Putting bounds on the capacity growth seems useful here and a few other spots, even if it doesn't necessarily have to be this tight.
```
if (old_cap > old_size) {
assert(real[idx].capacity() == old_cap);
} else {
assert(real[idx].capacity() > old_cap);
assert(real[idx].capacity() <= 2 * (old_cap + 1));
}
```
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626291871)
`s/Index/BufferIndex/` reduces my mental load fwiw
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626291871)
`s/Index/BufferIndex/` reduces my mental load fwiw
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626341902)
Calling `compare_fn` here seems more complete than picking and choosing when to call it before the final check
```Suggestion
if (!real.empty()) compare_fn(real[idx], sim[idx]);
if (non_full && command-- == 0) {
```
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626341902)
Calling `compare_fn` here seems more complete than picking and choosing when to call it before the final check
```Suggestion
if (!real.empty()) compare_fn(real[idx], sim[idx]);
if (non_full && command-- == 0) {
```