💬 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-2056917422)
Concept NACK
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. There is no benefit to doing this otherwise apparently and the biggest effect I see is that there will be fewer people able to contribute to that part of the code base. And this change alone will also take up a lot of review effort so it appears to be a clear negative,
...
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2056917422)
Concept NACK
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. There is no benefit to doing this otherwise apparently and the biggest effect I see is that there will be fewer people able to contribute to that part of the code base. And this change alone will also take up a lot of review effort so it appears to be a clear negative,
...
💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2056921899)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?
without focusing too much on the LN use-case, anytime you wanted to use something larger, you'd have to make sure you do so, f.e. when sweeping a max size commitment tx with penalty path. Basically you'll have to check if your tx is larger than 25k and switch if so.
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2056921899)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?
without focusing too much on the LN use-case, anytime you wanted to use something larger, you'd have to make sure you do so, f.e. when sweeping a max size commitment tx with penalty path. Basically you'll have to check if your tx is larger than 25k and switch if so.
💬 fanquake commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2056925244)
> Want to upstream the crc32 patch to match the others we have sitting there?
Sure. Opened a PR in our crc32c subtree fork: https://github.com/bitcoin-core/crc32c-subtree/pull/7, and one in Google upstream: https://github.com/google/crc32c/pull/64.
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2056925244)
> Want to upstream the crc32 patch to match the others we have sitting there?
Sure. Opened a PR in our crc32c subtree fork: https://github.com/bitcoin-core/crc32c-subtree/pull/7, and one in Google upstream: https://github.com/google/crc32c/pull/64.
🤔 glozow reviewed a pull request: "test: check disconnection when sending sendaddrv2 after verack"
(https://github.com/bitcoin/bitcoin/pull/29699#pullrequestreview-2001213888)
LGTM
(https://github.com/bitcoin/bitcoin/pull/29699#pullrequestreview-2001213888)
LGTM
💬 glozow commented on pull request "test: check disconnection when sending sendaddrv2 after verack":
(https://github.com/bitcoin/bitcoin/pull/29699#discussion_r1565855209)
(I think this is a good example of why we shouldn't use `assert_debug_log` when we're able to check the logic otherwise)
(https://github.com/bitcoin/bitcoin/pull/29699#discussion_r1565855209)
(I think this is a good example of why we shouldn't use `assert_debug_log` when we're able to check the logic otherwise)
🚀 glozow merged a pull request: "test: check disconnection when sending sendaddrv2 after verack"
(https://github.com/bitcoin/bitcoin/pull/29699)
(https://github.com/bitcoin/bitcoin/pull/29699)
🚀 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
...