💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031202)
This code is unused, the current value just affects testing, but it can literally be set to any positive integer, and the code will work (though at degraded performance for higher values). The real question is what to set the `max_cluster_count` value to in `MakeTxGraph()`, but I think that discussion belongs in #28676.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031202)
This code is unused, the current value just affects testing, but it can literally be set to any positive integer, and the code will work (though at degraded performance for higher values). The real question is what to set the `max_cluster_count` value to in `MakeTxGraph()`, but I think that discussion belongs in #28676.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031622)
I have largely rewritten this, and expanded comments. Please have a look and let me know if it is clearer now.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031622)
I have largely rewritten this, and expanded comments. Please have a look and let me know if it is clearer now.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031732)
Done.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1938031732)
Done.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628480335)
> But if we assume the miner was smart and used the available config options to optimize, they could have already reached 3,996,000 WU by setting `blockmaxweight` to 4,000,000 WU before this change, right?
No, the current behavior on master is that we always reserve `8,000 WU`, regardless of what `blockmaxweight` is. This is actually how I discovered the issue.
See this test https://github.com/ismaelsadeeq/bitcoin/commit/dbc17d1519247e366b39da54e87d0e866709d16a. It fails on master.
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628480335)
> But if we assume the miner was smart and used the available config options to optimize, they could have already reached 3,996,000 WU by setting `blockmaxweight` to 4,000,000 WU before this change, right?
No, the current behavior on master is that we always reserve `8,000 WU`, regardless of what `blockmaxweight` is. This is actually how I discovered the issue.
See this test https://github.com/ismaelsadeeq/bitcoin/commit/dbc17d1519247e366b39da54e87d0e866709d16a. It fails on master.
...
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628496788)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Ok, I am very confused now. In the release notes you write this:
```
- Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
`-blockreservedweight` to `4,000` to maintain the previous behaviour.
```
So you are saying setting `-blockreservedweight` to `4,000` will give you a block with `8000 WU` reserv
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628496788)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Ok, I am very confused now. In the release notes you write this:
```
- Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
`-blockreservedweight` to `4,000` to maintain the previous behaviour.
```
So you are saying setting `-blockreservedweight` to `4,000` will give you a block with `8000 WU` reserv
...
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1938048845)
Ok, you're right. I should not have framed this as a potential attack.
We maintain hard-coded hashes in depends for 2 reasons: To protect against real supply-chain attacks (Someone hijacks github and replaces tarballs with malicious ones) and for determinism (our sources can be described exactly).
The output of a depends build is locally deterministic based on the build id, which is comprised of the environment, toolchain, and source hashes. After this change, as far as I can tell, that's
...
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1938048845)
Ok, you're right. I should not have framed this as a potential attack.
We maintain hard-coded hashes in depends for 2 reasons: To protect against real supply-chain attacks (Someone hijacks github and replaces tarballs with malicious ones) and for determinism (our sources can be described exactly).
The output of a depends build is locally deterministic based on the build id, which is comprised of the environment, toolchain, and source hashes. After this change, as far as I can tell, that's
...
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054826)
Fixed
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054826)
Fixed
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054874)
Fixed
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054874)
Fixed
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054955)
Indeed, Fixed
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054955)
Indeed, Fixed
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055136)
I've removed this sentence since we allow `DEFAULT` and `ALL`.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055136)
I've removed this sentence since we allow `DEFAULT` and `ALL`.
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055244)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055244)
Done
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055281)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055281)
Done
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938059816)
These checks all occur inside of `migrate_and_get_rpc` now so they're redundant.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938059816)
These checks all occur inside of `migrate_and_get_rpc` now so they're redundant.
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938074664)
The verb "fill" suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think "Get" is still appropriate.
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938074664)
The verb "fill" suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think "Get" is still appropriate.
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938075462)
Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it's valid and correct.
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938075462)
Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it's valid and correct.
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938079255)
This PR is big enough already, I'd prefer to do that separately.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938079255)
This PR is big enough already, I'd prefer to do that separately.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628555787)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Maybe I am still not getting it but re-reading the release notes once more, I don't think they reflect this correctly. They seem to suggest that setting `-blockmaxweight` to `4,000,000` does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe a bit u
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628555787)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Maybe I am still not getting it but re-reading the release notes once more, I don't think they reflect this correctly. They seem to suggest that setting `-blockmaxweight` to `4,000,000` does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe a bit u
...
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938086462)
No... that's not at all how deserialization works. There's no constructors called, `prev_txid` is an already initialized `uint256`. Unserializing into it means that the data is overwritten. There's no `std::make_shared` called anywhere, nothing is constructed or initialized.
The call chain is `UnserializeFromVector` -> `UnserializeMany` -> `uint256::Unserialize()` -> `Unserialize(Stream& s, Span<B> span)` -> `<template Stream> read()`, and every `read(Span)` function checks that the stream ha
...
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938086462)
No... that's not at all how deserialization works. There's no constructors called, `prev_txid` is an already initialized `uint256`. Unserializing into it means that the data is overwritten. There's no `std::make_shared` called anywhere, nothing is constructed or initialized.
The call chain is `UnserializeFromVector` -> `UnserializeMany` -> `uint256::Unserialize()` -> `Unserialize(Stream& s, Span<B> span)` -> `<template Stream> read()`, and every `read(Span)` function checks that the stream ha
...
🤔 mzumsande reviewed a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2587949312)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2587949312)
Concept ACK
💬 mzumsande commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1938025806)
Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1938025806)
Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?