💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654636609)
Well, I don't even understand the big picture. Isn't the point of interfaces to have (possibly) multiple processes? How would one even take and release a lock in another process?
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654636609)
Well, I don't even understand the big picture. Isn't the point of interfaces to have (possibly) multiple processes? How would one even take and release a lock in another process?
💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654637548)
Actually that assert isn't the problem, it merely checks that the `chainman().ActiveChainstate()` and `chainman().ActiveChain().Tip()` that we passed in match.
The problem is deeper down the call stack, where `TestBlockValidity` calls `ConnectBlock`. That takes a `viewNew` argument which is `chainstate.CoinsTip()`, and our proposed `block`, and then it does `assert(hashPrevBlock == view.GetBestBlock());`.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654637548)
Actually that assert isn't the problem, it merely checks that the `chainman().ActiveChainstate()` and `chainman().ActiveChain().Tip()` that we passed in match.
The problem is deeper down the call stack, where `TestBlockValidity` calls `ConnectBlock`. That takes a `viewNew` argument which is `chainstate.CoinsTip()`, and our proposed `block`, and then it does `assert(hashPrevBlock == view.GetBestBlock());`.
✅ maflcko closed a pull request: "Docs improvements"
(https://github.com/bitcoin/bitcoin/pull/30337)
(https://github.com/bitcoin/bitcoin/pull/30337)
💬 maflcko commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191462423)
Ok, closing for now due to controversy. I think clear, correct and uncontroversial fixes are fine. However, if something is unclear, controversial and wrong, it seems better to avoid.
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191462423)
Ok, closing for now due to controversy. I think clear, correct and uncontroversial fixes are fine. However, if something is unclear, controversial and wrong, it seems better to avoid.
💬 maflcko commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191464078)
If you are looking for other contributions, see:
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once tha
...
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191464078)
If you are looking for other contributions, see:
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once tha
...
💬 romanz commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2191470515)
Many thanks!
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2191470515)
Many thanks!
💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654647663)
Good point, it doesn't make sense to require the lock. Will try something else.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654647663)
Good point, it doesn't make sense to require the lock. Will try something else.
💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654661048)
Pushed c8343e29a66082b37c37ee0ff8f7433b97eea9b1 to deal with this (replaces https://github.com/bitcoin/bitcoin/commit/18d6b0f86769f89738a3b08aa8152a8479cef5d4). This drops both locks from the RPC code.
An alternative that would actually prevent the tip from updating, rather than fail the RPC call, is to hold the lock even longer. But callers need to handle failure anyway since `testBlockValidity` can fail for other reasons.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654661048)
Pushed c8343e29a66082b37c37ee0ff8f7433b97eea9b1 to deal with this (replaces https://github.com/bitcoin/bitcoin/commit/18d6b0f86769f89738a3b08aa8152a8479cef5d4). This drops both locks from the RPC code.
An alternative that would actually prevent the tip from updating, rather than fail the RPC call, is to hold the lock even longer. But callers need to handle failure anyway since `testBlockValidity` can fail for other reasons.
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1654661980)
Will leave for now so as to not require reACKs for a house keeping change, but will reorder if I end up needing retouch. Thanks!
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1654661980)
Will leave for now so as to not require reACKs for a house keeping change, but will reorder if I end up needing retouch. Thanks!
💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654672807)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2:
```suggestion
assert_raises_rpc_error(-5, "Block not found", node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
```
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654672807)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2:
```suggestion
assert_raises_rpc_error(-5, "Block not found", node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
```
💬 Sjors commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2191508130)
> The actual sites on this domain have HSTS enabled with subdomain inclusion, although you have to visit those sites for the browser to know about that. I've also submitted it to the HSTS preload list.
Oh that's a good idea...
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2191508130)
> The actual sites on this domain have HSTS enabled with subdomain inclusion, although you have to visit those sites for the browser to know about that. I've also submitted it to the HSTS preload list.
Oh that's a good idea...
💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654674147)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2, I don't think this comment is necessary since the next line is clear and "self explanatory":
```suggestion
```
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654674147)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2, I don't think this comment is necessary since the next line is clear and "self explanatory":
```suggestion
```
💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2191519784)
Since calling `gettxoutsetinfo` with block hash depends on `coinstatsindex`. It would be simpler to address it on `feature_coinstatsindex`. It would be just one line, and would avoid the node restart and the function reordering.
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2191519784)
Since calling `gettxoutsetinfo` with block hash depends on `coinstatsindex`. It would be simpler to address it on `feature_coinstatsindex`. It would be just one line, and would avoid the node restart and the function reordering.
💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654687943)
`state` will be valid, no? So this may be confusing?
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654687943)
`state` will be valid, no? So this may be confusing?
💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654690210)
just a nit, because it only affects test-only code, but wanted to leave the comment.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654690210)
just a nit, because it only affects test-only code, but wanted to leave the comment.
💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191547359)
ACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)[e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191547359)
ACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)[e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
💬 AngusP commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654705968)
nit: In the RPC error you'll give `state.ToString()` as the failure reason, but if this tip check fails that'll be the default `BLOCK_RESULT_UNSET`.
Seems correct to fail if the tip has updated to avoid building a fork on an older block, but I can't see a `BlockValidationResult` that would make sense here other than maybe stretching the meaning of `BLOCK_INVALID_HEADER` -- perhaps you could add another that only makes sense in a templating context like `BLOCK_TEMPLTE_NOT_TIP` or wrap `BlockVa
...
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654705968)
nit: In the RPC error you'll give `state.ToString()` as the failure reason, but if this tip check fails that'll be the default `BLOCK_RESULT_UNSET`.
Seems correct to fail if the tip has updated to avoid building a fork on an older block, but I can't see a `BlockValidationResult` that would make sense here other than maybe stretching the meaning of `BLOCK_INVALID_HEADER` -- perhaps you could add another that only makes sense in a templating context like `BLOCK_TEMPLTE_NOT_TIP` or wrap `BlockVa
...
💬 AngusP commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654715485)
Currently if this happens as you said https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654637548 it looks like we'd have crashed which is fun
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654715485)
Currently if this happens as you said https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654637548 it looks like we'd have crashed which is fun
🤔 tdb3 reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2141646230)
Concept ACK
Left one minor comment.
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2141646230)
Concept ACK
Left one minor comment.
💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654715835)
Looks like this is checking for a block hash that doesn't exist after block hash for block 1 is reconsidered, which makes sense. Recommend adding `self.log.info()` to make this clearer to the reader.
```python
b1hash = node.getblockhash(1)
node.invalidateblock(b1hash)
...
self.log.info("Test gettxoutsetinfo returns the same result after invalidate/reconsider block")
node.reconsiderblock(b1hash)
```
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654715835)
Looks like this is checking for a block hash that doesn't exist after block hash for block 1 is reconsidered, which makes sense. Recommend adding `self.log.info()` to make this clearer to the reader.
```python
b1hash = node.getblockhash(1)
node.invalidateblock(b1hash)
...
self.log.info("Test gettxoutsetinfo returns the same result after invalidate/reconsider block")
node.reconsiderblock(b1hash)
```