🤔 fjahr reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2587690660)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2587690660)
Concept ACK
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937862071)
Couldn't you use `send_self_transfer_multi` instead?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937862071)
Couldn't you use `send_self_transfer_multi` instead?
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937893671)
nit: not sure if this will need to be reformatted later. Usually there is just one bullet per PR/change and it seems like this could be just one block of text. Just to take some editing headache from whoever manages the release...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937893671)
nit: not sure if this will need to be reformatted later. Usually there is just one bullet per PR/change and it seems like this could be just one block of text. Just to take some editing headache from whoever manages the release...
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937931886)
Why not check this against `max_block_weight` instead? The current behavior isn't catastrophic I guess because the clamping later silently fixes this. But it seems like the user is doing something wrong and in the case below we are throwing an error too instead of silently fixing it. Seems a little inconsistent at least.
```suggestion
if (block_reserved_weight > max_block_weight) {
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937931886)
Why not check this against `max_block_weight` instead? The current behavior isn't catastrophic I guess because the clamping later silently fixes this. But it seems like the user is doing something wrong and in the case below we are throwing an error too instead of silently fixing it. Seems a little inconsistent at least.
```suggestion
if (block_reserved_weight > max_block_weight) {
```
📝 davidgumberg opened a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774)
Fixes #31744
Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.
Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems a
...
(https://github.com/bitcoin/bitcoin/pull/31774)
Fixes #31744
Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.
Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems a
...
💬 achow101 commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2628328237)
ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2628328237)
ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628344352)
> This patch consolidates two 4000 WU reservations into one constant, and adjusts configuration parameters to maintain existing behavior. It allows more fine tuned control of block template creation and in particular will allow some mining pools to squeeze an extra 4000 WU of transaction data into new blocks.
Only an extra 2000 WU, right? Before the change setting setting blockmaxheight to max led to the actual minimum reserved 4000 WU. After the change there is the explicit minimum of 2000 W
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628344352)
> This patch consolidates two 4000 WU reservations into one constant, and adjusts configuration parameters to maintain existing behavior. It allows more fine tuned control of block template creation and in particular will allow some mining pools to squeeze an extra 4000 WU of transaction data into new blocks.
Only an extra 2000 WU, right? Before the change setting setting blockmaxheight to max led to the actual minimum reserved 4000 WU. After the change there is the explicit minimum of 2000 W
...
🚀 achow101 merged a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965)
(https://github.com/bitcoin/bitcoin/pull/30965)
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937951204)
My plan is to eventually remove the blockmaxweight option in a follow-up. See last sentence in [this comment](https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567472713). The behavior you describe is an edge case that is possible, but as you said, the clamping fixes it.
I think it is consistent to compare using `DEFAULT_BLOCK_MAX_WEIGHT` because, eventually, the block-reserved weight will be passed through the mining interface.
See the previous discussion on this [here](https:/
...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937951204)
My plan is to eventually remove the blockmaxweight option in a follow-up. See last sentence in [this comment](https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567472713). The behavior you describe is an edge case that is possible, but as you said, the clamping fixes it.
I think it is consistent to compare using `DEFAULT_BLOCK_MAX_WEIGHT` because, eventually, the block-reserved weight will be passed through the mining interface.
See the previous discussion on this [here](https:/
...
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937940475)
IIUC `send_self_transfer_multi` is creating and broadcasting a single transaction spending multiple UTXO's, in this case I want to create multiple transactions each spending a single UTXO.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937940475)
IIUC `send_self_transfer_multi` is creating and broadcasting a single transaction spending multiple UTXO's, in this case I want to create multiple transactions each spending a single UTXO.
💬 achow101 commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2628359009)
ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2628359009)
ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
👍 pinheadmz approved a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2587805051)
ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3
This adds a useful network testing utility without touching production code. I use both `StaticContentsSocket` and `DynSock` in testing an internal HTTP server in [this branch](https://github.com/pinheadmz/bitcoin/commits/http-rewrite/) and the API is clear and easy to use and produces expected results.
reviewed each commit, built and ran unit and functional tests on macos/arm
<details><summary>Show Signature</summary>
```
-----BEGIN PGP
...
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2587805051)
ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3
This adds a useful network testing utility without touching production code. I use both `StaticContentsSocket` and `DynSock` in testing an internal HTTP server in [this branch](https://github.com/pinheadmz/bitcoin/commits/http-rewrite/) and the API is clear and easy to use and produces expected results.
reviewed each commit, built and ran unit and functional tests on macos/arm
<details><summary>Show Signature</summary>
```
-----BEGIN PGP
...
💬 pinheadmz commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1937945365)
f1864148c4a091afd63be75bc1ff14ae93383523
unreachable right? you still need a return statement to satisfy the compiler?
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1937945365)
f1864148c4a091afd63be75bc1ff14ae93383523
unreachable right? you still need a return statement to satisfy the compiler?
💬 pinheadmz commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1937939627)
4b58d55878db55372d1b09de49c6caf363fe3c06
just noticing in the code-move the default argument was lost:
`Event* occurred = nullptr`
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1937939627)
4b58d55878db55372d1b09de49c6caf363fe3c06
just noticing in the code-move the default argument was lost:
`Event* occurred = nullptr`
🚀 achow101 merged a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909)
(https://github.com/bitcoin/bitcoin/pull/30909)
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628373528)
> Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see [NixOS/nixpkgs#359902](https://github.com/NixOS/nixpkgs/issues/359902)) that are more flexible and less monolithic than the depends system.
Perhaps this is the source of our breakdown. I would consider this an anti-feature. Or AT LEAST, as you've be
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628373528)
> Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see [NixOS/nixpkgs#359902](https://github.com/NixOS/nixpkgs/issues/359902)) that are more flexible and less monolithic than the depends system.
Perhaps this is the source of our breakdown. I would consider this an anti-feature. Or AT LEAST, as you've be
...
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628379396)
> Only an extra 2000 WU, right?
My understanding goes like this:
Imagine a miner that never needs more than 4000 for their coinbase, and has always used default settings. Their blocks have left behind 4000 empty space. After this patch, that same miner continuing to use default settings will continue to leave 4000 empty space per block. But now with the patch that miner has the option of filling that space with transaction data using `-blockreservedweight=4000`
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628379396)
> Only an extra 2000 WU, right?
My understanding goes like this:
Imagine a miner that never needs more than 4000 for their coinbase, and has always used default settings. Their blocks have left behind 4000 empty space. After this patch, that same miner continuing to use default settings will continue to leave 4000 empty space per block. But now with the patch that miner has the option of filling that space with transaction data using `-blockreservedweight=4000`
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937978920)
This unfortunately leaves us open to a supply-chain attack as it depends on the contents of the builders's environment. We'll need to commit to the hash of a deterministic tarball.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937978920)
This unfortunately leaves us open to a supply-chain attack as it depends on the contents of the builders's environment. We'll need to commit to the hash of a deterministic tarball.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628395523)
Thanks for your review, @pinheadmz and @fjahr.
I have answered the review questions and will address any nits when a retouch is needed.
@fjahr, think of it this way:
In the block assembler, we need to leave some space for the coinbase transaction, block header, and transaction count.
Previously, we reserved this space in multiple places `4000 WU` twice.
The `blockmaxweight` was described as `3996000 WU` because we intended to reserve space only once. However, due to duplicate
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628395523)
Thanks for your review, @pinheadmz and @fjahr.
I have answered the review questions and will address any nits when a retouch is needed.
@fjahr, think of it this way:
In the block assembler, we need to leave some space for the coinbase transaction, block header, and transaction count.
Previously, we reserved this space in multiple places `4000 WU` twice.
The `blockmaxweight` was described as `3996000 WU` because we intended to reserve space only once. However, due to duplicate
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628408316)
> I would consider this an anti-feature. Or AT LEAST, as you've been saying, one that can be added on top of the changes here once it's been discussed.
Well, if it is an anti-feature, it is a 5 line anti-feature and I am happy to drop it here. If I do this though I will probably make a separate PR to add it later, because I think it will be painful to develop and test upstream libmultiprocess changes without it.
> Anyway, I've pushed a demo of my changes here: https://github.com/theuni/bit
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628408316)
> I would consider this an anti-feature. Or AT LEAST, as you've been saying, one that can be added on top of the changes here once it's been discussed.
Well, if it is an anti-feature, it is a 5 line anti-feature and I am happy to drop it here. If I do this though I will probably make a separate PR to add it later, because I think it will be painful to develop and test upstream libmultiprocess changes without it.
> Anyway, I've pushed a demo of my changes here: https://github.com/theuni/bit
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933929575)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516
> [01452f8](https://github.com/bitcoin/bitcoin/commit/01452f8c0da58db549bc46d0cfa7de715344efc4): the Apple version of tar doesn't have `--sort`, see [chaincodelabs/libmultiprocess#139 (comment)](https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394)
Thanks, this is dropped
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933929575)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516
> [01452f8](https://github.com/bitcoin/bitcoin/commit/01452f8c0da58db549bc46d0cfa7de715344efc4): the Apple version of tar doesn't have `--sort`, see [chaincodelabs/libmultiprocess#139 (comment)](https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394)
Thanks, this is dropped