💬 Sjors commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503279705)
> Previously, they could access it with a single call, but after what you propose it will requires two calls.
Clients are already expect to call `getHeader()` to get the header. Combined with `getCoinbaseMerklePath()` and `getCoinbase()` lets them construct a template without having to download the full block.
In Stratum v2 clients only call `getBlock()` if the pool wants to inspect their proposed template (see `RequestTransactionData.Success` in sv2-tp). This happens later and currently invol
...
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503279705)
> Previously, they could access it with a single call, but after what you propose it will requires two calls.
Clients are already expect to call `getHeader()` to get the header. Combined with `getCoinbaseMerklePath()` and `getCoinbase()` lets them construct a template without having to download the full block.
In Stratum v2 clients only call `getBlock()` if the pool wants to inspect their proposed template (see `RequestTransactionData.Success` in sv2-tp). This happens later and currently invol
...
💬 maflcko commented on issue "fuzz: Use-of-uninitialized-value in evutil_inet_pton":
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503304087)
It was reproducible in our CI config once: https://issues.oss-fuzz.com/issues/42521668#comment5
But yeah, not sure if anyone cares.
(https://github.com/bitcoin/bitcoin/issues/27975#issuecomment-3503304087)
It was reproducible in our CI config once: https://issues.oss-fuzz.com/issues/42521668#comment5
But yeah, not sure if anyone cares.
💬 purpleKarrot commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503306025)
My position is that code like this is **not** something that belongs in a `CMakeLists.txt` file.
Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-2999638262).
As long as fun
...
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503306025)
My position is that code like this is **not** something that belongs in a `CMakeLists.txt` file.
Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-2999638262).
As long as fun
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504267008)
> do you plan on deprecating it first and then removing it in a later release?
No, because the interface is marked experimental I think it's fine to drop it in any release we want. That might be as soon as v31, but we can also do it later.
> your master should be in sync with master
`sv2-tp` is used by people who only run v30, so it has to support both master and v30.
It can already handle _new_ methods by just trying them and falling back to old methods when they're missing.
Rec
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504267008)
> do you plan on deprecating it first and then removing it in a later release?
No, because the interface is marked experimental I think it's fine to drop it in any release we want. That might be as soon as v31, but we can also do it later.
> your master should be in sync with master
`sv2-tp` is used by people who only run v30, so it has to support both master and v30.
It can already handle _new_ methods by just trying them and falling back to old methods when they're missing.
Rec
...
🤔 hodlinator reviewed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3364452726)
Agree this issue would be good to fix!
However also agree with https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3403430467 that logging a warning/error and doing some kind of higher level change instead of patching things up here would be preferable.
#### Higher level proposal
Add an earlier phase for `-reindex` where it rebuilds the headers chain/`BlockIndex` from blocks on disk, and if we're still below minimumchainwork, perform headerssync to populate the index prior to this
...
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3364452726)
Agree this issue would be good to fix!
However also agree with https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3403430467 that logging a warning/error and doing some kind of higher level change instead of patching things up here would be preferable.
#### Higher level proposal
Add an earlier phase for `-reindex` where it rebuilds the headers chain/`BlockIndex` from blocks on disk, and if we're still below minimumchainwork, perform headerssync to populate the index prior to this
...
💬 hodlinator commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2502875481)
(Prior to this PR, all `if`'s from here on down expected the current block to be an ancestor of the assumed valid block).
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2502875481)
(Prior to this PR, all `if`'s from here on down expected the current block to be an ancestor of the assumed valid block).
💬 hodlinator commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2503080190)
nit: if we keep these, `const bool` might be nicer, although the scope is small.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2503080190)
nit: if we keep these, `const bool` might be nicer, although the scope is small.
💬 hodlinator commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2451027360)
nit: Few tests report success, the test framework always outputs "Tests successful" if the test(s) it runs succeed already, so seems unnecessary.
```
₿ git grep 'self.log.info("Success")'
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.p
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2451027360)
nit: Few tests report success, the test framework always outputs "Tests successful" if the test(s) it runs succeed already, so seems unnecessary.
```
₿ git grep 'self.log.info("Success")'
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.py: self.log.info("Success")
test/functional/feature_pruning.p
...
💬 hodlinator commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2503513923)
remark:
Didn't get this at first - if *our block is an ancestor of `AssumedValidBlock()`*, why would either of:
a) it not being part of the best header chain
b) the best header chain being below minimum chainwork
...make us turn on script checking for this assumevalid ancestor?
It's been this way since assumevalid was introduced #9484 / e440ac7ef3b6f3ad1cd8fc7027cece40413202d9.
I guess a) could be to cover the case where a node runner is instructed by a malicious party to run with an `
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2503513923)
remark:
Didn't get this at first - if *our block is an ancestor of `AssumedValidBlock()`*, why would either of:
a) it not being part of the best header chain
b) the best header chain being below minimum chainwork
...make us turn on script checking for this assumevalid ancestor?
It's been this way since assumevalid was introduced #9484 / e440ac7ef3b6f3ad1cd8fc7027cece40413202d9.
I guess a) could be to cover the case where a node runner is instructed by a malicious party to run with an `
...
💬 Sjors commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503342616)
See inline discussion in https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504009667 about whether we should just go ahead and make breaking changes now, or pile them up until there's a bigger / more important breaking change.
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503342616)
See inline discussion in https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504009667 about whether we should just go ahead and make breaking changes now, or pile them up until there's a bigger / more important breaking change.
💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3503428587)
> Recent versions of sv2-tp all work against v30, so I haven't had to push people to switch to a newer version either. But we start making breaking changes, then I have to start instructing users of Bitcoin Core master to use specific newer versions of sv2-tp.
Thanks for the context. I still think it’s fine to do it together. You can have a major of your `sv2-tp` that works with `v30` and minor releases that includes additional improvements that the major releases that is compatible with v30.
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3503428587)
> Recent versions of sv2-tp all work against v30, so I haven't had to push people to switch to a newer version either. But we start making breaking changes, then I have to start instructing users of Bitcoin Core master to use specific newer versions of sv2-tp.
Thanks for the context. I still think it’s fine to do it together. You can have a major of your `sv2-tp` that works with `v30` and minor releases that includes additional improvements that the major releases that is compatible with v30.
...
💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504357664)
Thanks for the context. I still think it’s fine to do it together. You can have a major of your sv2-tp that works with v30 and minor releases that includes additional improvements that the major releases that is compatible with v30.
sv2-tp master should just be in sync with bitcoin/bitcoin people that want to run that should build master from source.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504357664)
Thanks for the context. I still think it’s fine to do it together. You can have a major of your sv2-tp that works with v30 and minor releases that includes additional improvements that the major releases that is compatible with v30.
sv2-tp master should just be in sync with bitcoin/bitcoin people that want to run that should build master from source.
💬 maflcko commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503504071)
> Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of `.pyc` files.
> If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503504071)
> Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.
I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of `.pyc` files.
> If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned [here](https://github.com/bitcoin
...
💬 m3dwards commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3503574153)
This is very cool.
Have I got the potential trade-offs correct here?
Positives:
- More portable - especially running a modern binary on an older linux
- Enable very small docker images
Downsides:
- Static glibc can struggle with resolvers and locale (although I don't think locale is an issue here) potentially undermining the portability benefit. I don't know if `--enable-static-nss` solves this? But as referenced in this [line](https://github.com/bitcoin/bitcoin/pull/25573/files#
...
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3503574153)
This is very cool.
Have I got the potential trade-offs correct here?
Positives:
- More portable - especially running a modern binary on an older linux
- Enable very small docker images
Downsides:
- Static glibc can struggle with resolvers and locale (although I don't think locale is an issue here) potentially undermining the portability benefit. I don't know if `--enable-static-nss` solves this? But as referenced in this [line](https://github.com/bitcoin/bitcoin/pull/25573/files#
...
💬 luke-jr commented on pull request "Comment out sensitive console commands in history to prevent re-execution":
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504521790)
Why does this need a signal/slot?
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504521790)
Why does this need a signal/slot?
💬 luke-jr commented on pull request "Comment out sensitive console commands in history to prevent re-execution":
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504531173)
Should probably check this before parsing (top of RPCConsole::on_lineEdit_returnPressed)
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504531173)
Should probably check this before parsing (top of RPCConsole::on_lineEdit_returnPressed)
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503617446)
Guix build for a02282d20c87:
- dist-archive
- `bitcoin-a02282d20c87.tar.gz`: `79d1ff42f480bd6894b1d44fd7a59bc7c3b17cf25cccae7fe10b7f540d37af10`
- aarch64-linux-gnu
- `bitcoin-a02282d20c87-aarch64-linux-gnu-debug.tar.gz`: `6e3b9a2e39e207d98c347ccc0a81dfc6c79484befa5dbbeaaccdd0b44b759e99`
- `bitcoin-a02282d20c87-aarch64-linux-gnu.tar.gz`: `827f5c070c2ee3c8e1eb96cfd6f05551e4a69a00e422f808c15840026c7a27fc`
- `SHA256SUMS.part`: `a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0
...
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503617446)
Guix build for a02282d20c87:
- dist-archive
- `bitcoin-a02282d20c87.tar.gz`: `79d1ff42f480bd6894b1d44fd7a59bc7c3b17cf25cccae7fe10b7f540d37af10`
- aarch64-linux-gnu
- `bitcoin-a02282d20c87-aarch64-linux-gnu-debug.tar.gz`: `6e3b9a2e39e207d98c347ccc0a81dfc6c79484befa5dbbeaaccdd0b44b759e99`
- `bitcoin-a02282d20c87-aarch64-linux-gnu.tar.gz`: `827f5c070c2ee3c8e1eb96cfd6f05551e4a69a00e422f808c15840026c7a27fc`
- `SHA256SUMS.part`: `a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504559542)
In the longer run it probably makes sense for `sv2-tp` to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504559542)
In the longer run it probably makes sense for `sv2-tp` to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.
📝 stickies-v opened a pull request: "kernel: trim Chain interface"
(https://github.com/bitcoin/bitcoin/pull/33820)
Removes `btck_chain_get_genesis` and `btck_chain_get_tip`.
They are trivially replaced with `btck_chain_get_by_height` (as indicated in the updated `bitcoinkernel_wrapper.h`, so I think it makes sense to trim the interface.
For `btck_chain_get_tip`: on `master` we don't provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn't seem like a regression to me.
(https://github.com/bitcoin/bitcoin/pull/33820)
Removes `btck_chain_get_genesis` and `btck_chain_get_tip`.
They are trivially replaced with `btck_chain_get_by_height` (as indicated in the updated `bitcoinkernel_wrapper.h`, so I think it makes sense to trim the interface.
For `btck_chain_get_tip`: on `master` we don't provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn't seem like a regression to me.
💬 average-gary commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503637844)
@l0rinc I applied your patch in commits that I hope are focused enough and made you co-author. Thank you.
I'll find some time to review the rest of the comments and address as needed after this patch.
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503637844)
@l0rinc I applied your patch in commits that I hope are focused enough and made you co-author. Thank you.
I'll find some time to review the rest of the comments and address as needed after this patch.