💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626076767)
nit: The comment could be updated to match the current action, e.g. `# Renaming the block file to induce unsuccessful block read`.
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626076767)
nit: The comment could be updated to match the current action, e.g. `# Renaming the block file to induce unsuccessful block read`.
💬 ffrediani commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147939514)
I wish to contribute this way, having a proper and structured discussion about a topic which is still an issue. If you want to suggest other ways to contribute it is fine, but I hope you are not making it a must on the way I must contribute.
Creating this issue contributes to raise awareness to the issue (which currently exists), to discuss possible solution and have a solution implemented for it.
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147939514)
I wish to contribute this way, having a proper and structured discussion about a topic which is still an issue. If you want to suggest other ways to contribute it is fine, but I hope you are not making it a must on the way I must contribute.
Creating this issue contributes to raise awareness to the issue (which currently exists), to discuss possible solution and have a solution implemented for it.
💬 ffrediani commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30224#issuecomment-2147949569)
@pinheadmz if there are alternatives ways and strategies why just stay with what is there right now and not improve it for these scenarios ?
Seems some developers in this project don't like to have issues opened they can't or are unwilling to work on and arbitrarily close them. I still disagree with this autocratic behaviour and that it should be discussed elsewhere. Seems more a personal preference of some rather than how it really is.
This is a feature request and as such has a place in
...
(https://github.com/bitcoin/bitcoin/issues/30224#issuecomment-2147949569)
@pinheadmz if there are alternatives ways and strategies why just stay with what is there right now and not improve it for these scenarios ?
Seems some developers in this project don't like to have issues opened they can't or are unwilling to work on and arbitrarily close them. I still disagree with this autocratic behaviour and that it should be discussed elsewhere. Seems more a personal preference of some rather than how it really is.
This is a feature request and as such has a place in
...
💬 maflcko commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147949594)
Can you explain how your discussion is different from the assumeutxo discussions already happening? I don't think it makes sense to have duplicate and redundant issues.
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2147949594)
Can you explain how your discussion is different from the assumeutxo discussions already happening? I don't think it makes sense to have duplicate and redundant issues.
📝 aaservice opened a pull request: "Update and rename bitcoinkernel.cpp to bitcoinkernel.cppx"
(https://github.com/bitcoin/bitcoin/pull/30225)
Q
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that i
...
(https://github.com/bitcoin/bitcoin/pull/30225)
Q
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that i
...
✅ fanquake closed a pull request: "Update and rename bitcoinkernel.cpp to bitcoinkernel.cppx"
(https://github.com/bitcoin/bitcoin/pull/30225)
(https://github.com/bitcoin/bitcoin/pull/30225)
📝 fanquake locked a pull request: "Update and rename bitcoinkernel.cpp to bitcoinkernel.cppx"
(https://github.com/bitcoin/bitcoin/pull/30225)
Q
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that i
...
(https://github.com/bitcoin/bitcoin/pull/30225)
Q
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that i
...
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2148033169)
Another thing you could try to debug this further is to put a swapfile, and the datadir on the same AWS gp3 SSD filesystem.
I am happy to create an AWS account to test this, but it would be good if there was a single (bash) script, which can be deployed to AWS, so that it is easy for anyone to reproduce your exact setup.
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2148033169)
Another thing you could try to debug this further is to put a swapfile, and the datadir on the same AWS gp3 SSD filesystem.
I am happy to create an AWS account to test this, but it would be good if there was a single (bash) script, which can be deployed to AWS, so that it is easy for anyone to reproduce your exact setup.
💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2148037183)
concept ACK
requested changes were included, will continue review
`git range-diff master 18ec3fc c99063e`
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2148037183)
concept ACK
requested changes were included, will continue review
`git range-diff master 18ec3fc c99063e`
💬 sipa commented on pull request "feefrac: 128-bit multiply support in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29758#issuecomment-2148067460)
@hebasto Would you mind benchmarking with smaller realistic numbers (say fee and size both between 0 and 2^30)? If the top limb is equal, it's possible the naive code does worse.
(https://github.com/bitcoin/bitcoin/pull/29758#issuecomment-2148067460)
@hebasto Would you mind benchmarking with smaller realistic numbers (say fee and size both between 0 and 2^30)? If the top limb is equal, it's possible the naive code does worse.
👍 ryanofsky approved a pull request: "Introduce Miner interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2096918003)
Code review ACK 87ba70a2731951a16432ca2d8d55c62bad87dd41
re: https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2140187315
> Let me know if I should expand the interface to cover more of `rpc/mining.cpp` or stick to this for now.
I think it's just important for the interface to be usable for stratum code so is not too tied to node internals. It should be ok for RPC code to go around the interface and not use it for everything.
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2096918003)
Code review ACK 87ba70a2731951a16432ca2d8d55c62bad87dd41
re: https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2140187315
> Let me know if I should expand the interface to cover more of `rpc/mining.cpp` or stick to this for now.
I think it's just important for the interface to be usable for stratum code so is not too tied to node internals. It should be ok for RPC code to go around the interface and not use it for everything.
💬 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.