💬 TheCharlatan commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880837705)
Can the full diff be reproduced with `run-clang-tidy -fix`? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880837705)
Can the full diff be reproduced with `run-clang-tidy -fix`? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.
💬 aureleoules commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880843285)
> Can the full diff be reproduced with run-clang-tidy -fix? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.
Sorry, my initial push did not pass the tidy check because it seems the `-fix` option does not fix code in macros (all the asserts, Assume, etc.) so I manually fixed these in a second push. This may be the reason why you are not getting the same diff as me.
I don't have the specific command I used on hand but I used the
...
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880843285)
> Can the full diff be reproduced with run-clang-tidy -fix? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.
Sorry, my initial push did not pass the tidy check because it seems the `-fix` option does not fix code in macros (all the asserts, Assume, etc.) so I manually fixed these in a second push. This may be the reason why you are not getting the same diff as me.
I don't have the specific command I used on hand but I used the
...
💬 fanquake commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337)
Pushed up the patches into [my PR](https://github.com/Homebrew/homebrew-core/pull/156745), it has fixed some of the test failures, but not all: https://github.com/Homebrew/homebrew-core/actions/runs/7446626418/job/20257419440?pr=156745#step:4:82.
```bash
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/26.0/bin/test_bitcoin
Running 585 test cases...
test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [1 != 0]
test/serfloat_tests.cpp
...
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337)
Pushed up the patches into [my PR](https://github.com/Homebrew/homebrew-core/pull/156745), it has fixed some of the test failures, but not all: https://github.com/Homebrew/homebrew-core/actions/runs/7446626418/job/20257419440?pr=156745#step:4:82.
```bash
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/26.0/bin/test_bitcoin
Running 585 test cases...
test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [1 != 0]
test/serfloat_tests.cpp
...
💬 fanquake commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1880865889)
> Again, it would be good to have steps to reproduce locally.
See https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337.
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1880865889)
> Again, it would be good to have steps to reproduce locally.
See https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337.
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1880891111)
Sorry for the slow response. I was away buit am now back.
Thanks for the review, I agree that setting special bits is uninteresting for cookie files (I was really trying to keep the subfunctions generic in case we wanted to re-use them elsewhere). I've now removed ability to set special bits.
I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same per
...
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1880891111)
Sorry for the slow response. I was away buit am now back.
Thanks for the review, I agree that setting special bits is uninteresting for cookie files (I was really trying to keep the subfunctions generic in case we wanted to re-use them elsewhere). I've now removed ability to set special bits.
I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same per
...
🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1808799983)
Combed through the code a bit more. Generally approach ACK for "cluster 2 replace cluster 2" idea.
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1808799983)
Combed through the code a bit more. Generally approach ACK for "cluster 2 replace cluster 2" idea.
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444482970)
Looks correct. If you're willing to entertain a suggestion to make it more readable, I think the following logic is easier to follow and the strings less ambiguous. Since "desc count' and "anc count" usually mean inclusive of tx, it's confusing if it's different in this function.
```suggestion
// Ancestor and descendant counts are inclusive of the tx itself.
const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
const auto descendant_count{direct_confl
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444482970)
Looks correct. If you're willing to entertain a suggestion to make it more readable, I think the following logic is easier to follow and the strings less ambiguous. Since "desc count' and "anc count" usually mean inclusive of tx, it's confusing if it's different in this function.
```suggestion
// Ancestor and descendant counts are inclusive of the tx itself.
const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
const auto descendant_count{direct_confl
...
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444457214)
Maybe add "Each entry must be part of a cluster with size <=2."
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444457214)
Maybe add "Each entry must be part of a cluster with size <=2."
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444555154)
Just realized this log is pretty nonsensical for package RBF, and also whenever there are multiple transactions being replaced in by a single one.
Maybe this log should be:
```
LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",
it->GetTx().GetHash().ToString(),
it->GetTx().GetWitnessHash().ToString(),
it->GetFee(),
it->GetTxSize(),
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444555154)
Just realized this log is pretty nonsensical for package RBF, and also whenever there are multiple transactions being replaced in by a single one.
Maybe this log should be:
```
LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",
it->GetTx().GetHash().ToString(),
it->GetTx().GetWitnessHash().ToString(),
it->GetFee(),
it->GetTxSize(),
...
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444505904)
56e2fb281a0e62cb70e610370c2dce9e79bc05f0
We copy this list into every transaction's `MempoolAcceptResult` (which does makes sense since both the parent and the child participated in the replacement of these txns - that's probably helpful for showing package RBFs in the RPC results). But the duplicates are a problem when we add to `vExtraTxnForCompact` in p2p:
https://github.com/bitcoin/bitcoin/blob/04b9df0f9fd95e907b2e8bf823d63e9dfb37b4ad/src/net_processing.cpp#L4254-L4256
Possible soluti
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444505904)
56e2fb281a0e62cb70e610370c2dce9e79bc05f0
We copy this list into every transaction's `MempoolAcceptResult` (which does makes sense since both the parent and the child participated in the replacement of these txns - that's probably helpful for showing package RBFs in the RPC results). But the duplicates are a problem when we add to `vExtraTxnForCompact` in p2p:
https://github.com/bitcoin/bitcoin/blob/04b9df0f9fd95e907b2e8bf823d63e9dfb37b4ad/src/net_processing.cpp#L4254-L4256
Possible soluti
...
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444492292)
nit: good place to use annotations to double-check we're passing in the right fees:
```suggestion
if (const auto err_string{PaysForRBF(/*original_fees=*/m_conflicting_fees, /*replacement_fees=*/m_total_modified_fees, m_total_vsize,
m_pool.m_incremental_relay_feerate, child_hash)}) {
```
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444492292)
nit: good place to use annotations to double-check we're passing in the right fees:
```suggestion
if (const auto err_string{PaysForRBF(/*original_fees=*/m_conflicting_fees, /*replacement_fees=*/m_total_modified_fees, m_total_vsize,
m_pool.m_incremental_relay_feerate, child_hash)}) {
```
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444495751)
Maybe add a test case for `CheckConflictTopology` where our 1 parent has 2 children, or our 1 child has 2 parents
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444495751)
Maybe add a test case for `CheckConflictTopology` where our 1 parent has 2 children, or our 1 child has 2 parents
💬 kristapsk commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881022140)
> I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`, t
...
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881022140)
> I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`, t
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444669440)
dealing with duplicates seems strictly superior, will take a look, thanks!
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444669440)
dealing with duplicates seems strictly superior, will take a look, thanks!
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881049934)
> > I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`,
...
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881049934)
> > I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`,
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444673561)
the more work solutions would be to refactor everything to have a "subpackage eval" state, but juice is probably not worth the squeeze until cluster mempool, where "chunk eval" is likely the natural boundary
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444673561)
the more work solutions would be to refactor everything to have a "subpackage eval" state, but juice is probably not worth the squeeze until cluster mempool, where "chunk eval" is likely the natural boundary
💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1881052463)
@fanquake What if you drop the last commit here?
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1881052463)
@fanquake What if you drop the last commit here?
💬 kristapsk commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881053840)
> I don't think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense?
Ahh, yes, `400`, `440`, `444`.
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881053840)
> I don't think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense?
Ahh, yes, `400`, `440`, `444`.
💬 hebasto commented on issue "qa: No access to `debug.log` in functional tests on Windows":
(https://github.com/bitcoin/bitcoin/issues/28529#issuecomment-1881119075)
https://github.com/bitcoin/bitcoin/actions/runs/7433919780/job/20227517742:
```
2024-01-06T21:17:20.926000Z TestFramework (INFO): Tests successful
stderr:
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin/test/functional/tool_utxo_to_sqlite.py", line 111, in <module>
UtxoToSqliteTest().main()
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 154, in main
exit_code = self.shutdown()
^^^^^^^^^^^^^^^
...
(https://github.com/bitcoin/bitcoin/issues/28529#issuecomment-1881119075)
https://github.com/bitcoin/bitcoin/actions/runs/7433919780/job/20227517742:
```
2024-01-06T21:17:20.926000Z TestFramework (INFO): Tests successful
stderr:
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin/test/functional/tool_utxo_to_sqlite.py", line 111, in <module>
UtxoToSqliteTest().main()
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 154, in main
exit_code = self.shutdown()
^^^^^^^^^^^^^^^
...
💬 apoelstra commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1881121970)
> Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?
For my part I would rather have a library function than a CLI tool.
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1881121970)
> Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?
For my part I would rather have a library function than a CLI tool.