🚀 glozow merged a pull request: "rpc, bugfix: Enforce maximum value for setmocktime"
(https://github.com/bitcoin/bitcoin/pull/29869)
(https://github.com/bitcoin/bitcoin/pull/29869)
💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056954530)
> As I have said before, I think there needs to be a discussion about using more Rust just for the sake of using more Rust in the project. The only motivation given so far is that @maflcko said that it could be done.
Not sure where you get that from, but I never said that using more Rust for the sake of using more Rust can be done. I said something else:
* `I *don't* think moving all Python code to Rust makes sense.` https://www.github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022526794
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056954530)
> As I have said before, I think there needs to be a discussion about using more Rust just for the sake of using more Rust in the project. The only motivation given so far is that @maflcko said that it could be done.
Not sure where you get that from, but I never said that using more Rust for the sake of using more Rust can be done. I said something else:
* `I *don't* think moving all Python code to Rust makes sense.` https://www.github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022526794
...
💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056968267)
> Not sure where you get that from
"an alternative might be to rewrite the 3 lint checks to rust" https://github.com/bitcoin/bitcoin/pull/29479#pullrequestreview-1957109397 That is what @davidgumberg has given as the motivation in his description. It does not matter what you have said in other places because I have not been talking about that. I only talked about your comment that the author took as the motivation for this PR.
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056968267)
> Not sure where you get that from
"an alternative might be to rewrite the 3 lint checks to rust" https://github.com/bitcoin/bitcoin/pull/29479#pullrequestreview-1957109397 That is what @davidgumberg has given as the motivation in his description. It does not matter what you have said in other places because I have not been talking about that. I only talked about your comment that the author took as the motivation for this PR.
💬 theStack commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2056969396)
> > remove unused cpp-subprocess options
>
> Are all remaining options used?
From the 12 available cpp-subprocess options we currently only use 4 (namely `executable`, `input`, `output` and `error`):
https://github.com/bitcoin/bitcoin/blob/22c86140f8fe4f13b7b5415ff62922e497fd4948/src/common/run_command.cpp#L29
I.e. the other 8 are unused. This PR currently removes 3 of the 8 unused ones -- the remaining 5 (`cwd`, `bufsize`, `environment`, `close_fds`, `session_leader`) seemed to be commo
...
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2056969396)
> > remove unused cpp-subprocess options
>
> Are all remaining options used?
From the 12 available cpp-subprocess options we currently only use 4 (namely `executable`, `input`, `output` and `error`):
https://github.com/bitcoin/bitcoin/blob/22c86140f8fe4f13b7b5415ff62922e497fd4948/src/common/run_command.cpp#L29
I.e. the other 8 are unused. This PR currently removes 3 of the 8 unused ones -- the remaining 5 (`cwd`, `bufsize`, `environment`, `close_fds`, `session_leader`) seemed to be commo
...
💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1565878704)
For this function, I wasn't sure which rounding type to use. That's why I opted to keep the implementation and just change to Decimal
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1565878704)
For this function, I wasn't sure which rounding type to use. That's why I opted to keep the implementation and just change to Decimal
🤔 pablomartin4btc reviewed a pull request: "Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one"
(https://github.com/bitcoin/bitcoin/pull/29658#pullrequestreview-2001282587)
I think this should be moved to the QT gui [repo](https://github.com/bitcoin-core/gui).
(https://github.com/bitcoin/bitcoin/pull/29658#pullrequestreview-2001282587)
I think this should be moved to the QT gui [repo](https://github.com/bitcoin-core/gui).
👍 pablomartin4btc approved a pull request: "contrib: list other binaries in manpage output"
(https://github.com/bitcoin/bitcoin/pull/29585#pullrequestreview-2001289630)
utACK 7c3ac598dd9a1f1a506c4931249ff6c9f1c949ba
(https://github.com/bitcoin/bitcoin/pull/29585#pullrequestreview-2001289630)
utACK 7c3ac598dd9a1f1a506c4931249ff6c9f1c949ba
💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1565909454)
Because I cannot directly multiply a` Decimal object (fee_increment)` with a `float (the result of 1.1892 ** random.randint(0, 28))`
results in an aror
**TypeError: unsupported operand type(s) for *: 'decimal.Decimal' and 'float'**
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1565909454)
Because I cannot directly multiply a` Decimal object (fee_increment)` with a `float (the result of 1.1892 ** random.randint(0, 28))`
results in an aror
**TypeError: unsupported operand type(s) for *: 'decimal.Decimal' and 'float'**
📝 instagibbs opened a pull request: "fuzz: explicitly cap the vsize of RBFs for diagram checks"
(https://github.com/bitcoin/bitcoin/pull/29879)
In master we are hitting a case where vsize transactions much larger than max standard size are causing an overflow in not-yet-exposed RBF diagram checking code: https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049220195
`ConsumeTxMemPoolEntry` is creating entries with tens of thousands of sigops cost, causing the resulting RBFs to be "overly large".
To fix this I gate the resulting entries and bail if too large for mempool entry. I also add a number of `static_assert` checks wh
...
(https://github.com/bitcoin/bitcoin/pull/29879)
In master we are hitting a case where vsize transactions much larger than max standard size are causing an overflow in not-yet-exposed RBF diagram checking code: https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049220195
`ConsumeTxMemPoolEntry` is creating entries with tens of thousands of sigops cost, causing the resulting RBFs to be "overly large".
To fix this I gate the resulting entries and bail if too large for mempool entry. I also add a number of `static_assert` checks wh
...
💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057038648)
That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place). Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057038648)
That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place). Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
💬 laanwj commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2057039065)
> For example, setting environment variables seems like it could be useful, but the code is [quite complex for Windows](https://github.com/bitcoin/bitcoin/blob/07720b1cdd77399f32124641dbe1dd267eb0cf8b/src/util/subprocess.hpp#L309-L400).
Agree that that seems useful. I vaguely remember we even had a use-case for that once, but don't remember it clearly, we did give it up because it was too hard to do in windows.
> Good question. Note that close_fds is currently not removed.
Oh, whoops,
...
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2057039065)
> For example, setting environment variables seems like it could be useful, but the code is [quite complex for Windows](https://github.com/bitcoin/bitcoin/blob/07720b1cdd77399f32124641dbe1dd267eb0cf8b/src/util/subprocess.hpp#L309-L400).
Agree that that seems useful. I vaguely remember we even had a use-case for that once, but don't remember it clearly, we did give it up because it was too hard to do in windows.
> Good question. Note that close_fds is currently not removed.
Oh, whoops,
...
💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057047583)
> That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place).
Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
> Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
Well, my impression is th
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057047583)
> That comment says `so that they can use the already existing function` as the motivation. (This is the issue that was attempted to be solved and why the other pull request was created in the first place).
Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
> Re-reading my comment, I don't get the impression that it said "using more Rust just for the sake of using more Rust in the project can be done".
Well, my impression is th
...
💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057069224)
> Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
My understanding is that the original PR didn't actually fix the issue, so the problem remains. For example, `crypto/ctaes` is a subtree, but `test/lint/lint_ignore_dirs.py` does not list it. Also, `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` don't use `test/lint/lint_ignore_dirs.py`.
Let me know if I am missing something. If the issue is ind
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057069224)
> Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
My understanding is that the original PR didn't actually fix the issue, so the problem remains. For example, `crypto/ctaes` is a subtree, but `test/lint/lint_ignore_dirs.py` does not list it. Also, `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` don't use `test/lint/lint_ignore_dirs.py`.
Let me know if I am missing something. If the issue is ind
...
💬 fjahr commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057113596)
> For example, crypto/ctaes is a subtree, but test/lint/lint_ignore_dirs.py does not list it. Also, test/lint/lint-locale-dependence.py and test/lint/lint-python-utf8-encoding.py don't use test/lint/lint_ignore_dirs.py.
Right, these are behavior changes that I did not see as part of the motivation of the original PR, as the title says it was a refactor. So I don't think this is something that was missed, it was just not the goal. It should be made more clear that this is the main motivation
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2057113596)
> For example, crypto/ctaes is a subtree, but test/lint/lint_ignore_dirs.py does not list it. Also, test/lint/lint-locale-dependence.py and test/lint/lint-python-utf8-encoding.py don't use test/lint/lint_ignore_dirs.py.
Right, these are behavior changes that I did not see as part of the motivation of the original PR, as the title says it was a refactor. So I don't think this is something that was missed, it was just not the goal. It should be made more clear that this is the main motivation
...
💬 fanquake commented on pull request "depends: build miniupnpc with CMake":
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2057119905)
Pushed for builds now that the mirrors are back up.
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2057119905)
Pushed for builds now that the mirrors are back up.
💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2057124280)
Looks like there could be issues with expat, CMake and 32-bit builds:
* https://cirrus-ci.com/task/6695396838211584
* https://cirrus-ci.com/task/5850971908079616
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2057124280)
Looks like there could be issues with expat, CMake and 32-bit builds:
* https://cirrus-ci.com/task/6695396838211584
* https://cirrus-ci.com/task/5850971908079616
💬 achow101 commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057136484)
> Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
Yes, I did that. I could not find any benchmark that had a significant difference.
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2057136484)
> Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
Yes, I did that. I could not find any benchmark that had a significant difference.
💬 fjahr commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057150785)
@sipa The description still lists "Add more tests" as an open todo. Did you still want to address this before this could be merged? I think we have ok-ish coverage of MuHash but if you think something should be added could you tell us what you had in mind? Are there maybe test vectors we can port over?
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057150785)
@sipa The description still lists "Add more tests" as an open todo. Did you still want to address this before this could be merged? I think we have ok-ish coverage of MuHash but if you think something should be added could you tell us what you had in mind? Are there maybe test vectors we can port over?
💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057153942)
@fjahr I've dropped the TODO. Feel free to contribute tests of course if you feel that's helpful.
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2057153942)
@fjahr I've dropped the TODO. Feel free to contribute tests of course if you feel that's helpful.
💬 instagibbs commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566001308)
this is hopefully a conservative over-estimate since clusters should only be 101kvB total each
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566001308)
this is hopefully a conservative over-estimate since clusters should only be 101kvB total each