🚀 fanquake merged a pull request: "rest: don't copy data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321)
(https://github.com/bitcoin/bitcoin/pull/30321)
💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654581962)
> I pushed [1f0559e](https://github.com/bitcoin/bitcoin/commit/1f0559e9954253c23af7ee9b49e57eb3d7a45bea) to drop it from `createNewBlock`. Will look at the other occurances too.
The commit referenced in this commit message is wrong.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654581962)
> I pushed [1f0559e](https://github.com/bitcoin/bitcoin/commit/1f0559e9954253c23af7ee9b49e57eb3d7a45bea) to drop it from `createNewBlock`. Will look at the other occurances too.
The commit referenced in this commit message is wrong.
🚀 fanquake merged a pull request: "chainparams: Add achow101 DNS seeder"
(https://github.com/bitcoin/bitcoin/pull/30007)
(https://github.com/bitcoin/bitcoin/pull/30007)
💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654603274)
The lock at line 390 has been there from the introduction of the `generateblock` RPC call in #17693. I don't see any discussion of it in the reviews.
I think it's because `assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());` inside of `TestBlockValidity`. Added d23968e0fb6e1d7c5d19b07722796181d6e04f5b to explain that, and also make `testBlockValidity` require the lock.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654603274)
The lock at line 390 has been there from the introduction of the `generateblock` RPC call in #17693. I don't see any discussion of it in the reviews.
I think it's because `assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());` inside of `TestBlockValidity`. Added d23968e0fb6e1d7c5d19b07722796181d6e04f5b to explain that, and also make `testBlockValidity` require the lock.
📝 willcl-ark opened a pull request: "WIP: Permit Combiner to strip bip32_deriv information"
(https://github.com/bitcoin/bitcoin/pull/30341)
Related to https://github.com/bitcoin/bitcoin/issues/30294
Looking for approach (N)ACK.
Previously setting the `bip32derivs` flag to false with the `walletprocesspsbt` RPC correctly does not include `bip32_derivs` for inputs in the PSBT, but does include `bip32_derivs` for outputs.
User may want to actively strip all bip32 derivation paths from a PSBT for privacy reasons however. It may make sense to do this after signing your inputs and outputs during a manual coinjoin as demonstrated
...
(https://github.com/bitcoin/bitcoin/pull/30341)
Related to https://github.com/bitcoin/bitcoin/issues/30294
Looking for approach (N)ACK.
Previously setting the `bip32derivs` flag to false with the `walletprocesspsbt` RPC correctly does not include `bip32_derivs` for inputs in the PSBT, but does include `bip32_derivs` for outputs.
User may want to actively strip all bip32 derivation paths from a PSBT for privacy reasons however. It may make sense to do this after signing your inputs and outputs during a manual coinjoin as demonstrated
...
💬 willcl-ark commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2191419205)
Ok I see, what you are actually after is an active "eraser", so that you can perform the signing (perhaps on an HWW) with derivation paths included, but then not have them present when passing on to another party. I think this works better for the `Combiner` role, personally.
Your workflow would then work something more like this:
> Create a psbt with bip32derivs included, pass psbt to offline signer which signs inputs.
>
> Pass signed psbt to `combinepsbt` with _some flag_ set to `false
...
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2191419205)
Ok I see, what you are actually after is an active "eraser", so that you can perform the signing (perhaps on an HWW) with derivation paths included, but then not have them present when passing on to another party. I think this works better for the `Combiner` role, personally.
Your workflow would then work something more like this:
> Create a psbt with bip32derivs included, pass psbt to offline signer which signs inputs.
>
> Pass signed psbt to `combinepsbt` with _some flag_ set to `false
...
💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654615945)
Lock annotations in the cpp file will be ignored by the compiler, because it usually only sees the `.h` file.
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654615945)
Lock annotations in the cpp file will be ignored by the compiler, because it usually only sees the `.h` file.
💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619461)
Done.
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619461)
Done.
💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619596)
Done.
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619596)
Done.
💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191434797)
Force-pushed:
- I added a commit to change some comments to `self.log.info`
- Addressed: https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653913528 and https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653781334
Thanks, @kevkevinpal and @tdb3.
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191434797)
Force-pushed:
- I added a commit to change some comments to `self.log.info`
- Addressed: https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653913528 and https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653781334
Thanks, @kevkevinpal and @tdb3.
💬 willcl-ark commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191448472)
Is this even worth keeping open? By my count only 2 of the 8 commits are arguably correct, and it hardly seems worth the review/merge effort at that point...
Checking @nnsW3 [GitHub](https://github.com/nnsW3), it appears that they only provide fly-by spelling changes to a large number of repos, to what ends, who knows?
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191448472)
Is this even worth keeping open? By my count only 2 of the 8 commits are arguably correct, and it hardly seems worth the review/merge effort at that point...
Checking @nnsW3 [GitHub](https://github.com/nnsW3), it appears that they only provide fly-by spelling changes to a large number of repos, to what ends, who knows?
💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654633765)
I initially added it only to the header, but that resulted in:
```
node/interfaces.cpp:875:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
AssertLockHeld(cs_main);
^
```
Should I add it in both? Or declare the `override` first (which would need a new header).
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654633765)
I initially added it only to the header, but that resulted in:
```
node/interfaces.cpp:875:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
AssertLockHeld(cs_main);
^
```
Should I add it in both? Or declare the `override` first (which would need a new header).
💬 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.