💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461526010)
Why 5?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461526010)
Why 5?
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903529160)
@maflcko that comment refers to running CI for checking my own work. But what this pull request enables is running CI for pull requests, which may be from myself but also from others.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903529160)
@maflcko that comment refers to running CI for checking my own work. But what this pull request enables is running CI for pull requests, which may be from myself but also from others.
💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903550684)
I like the idea of operating on `CKey` directly. I'll try to add that, though not sure how to implement that securely.
The keys uses in #28983 are not as important as wallet keys, but if we add a generic method to store a `CKey` on disk, then it should do so securely - so that future devs using that function don't themselves in the foot. At the least the current `std::vector<unsigned char>>` does not pretend to be secure.
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903550684)
I like the idea of operating on `CKey` directly. I'll try to add that, though not sure how to implement that securely.
The keys uses in #28983 are not as important as wallet keys, but if we add a generic method to store a `CKey` on disk, then it should do so securely - so that future devs using that function don't themselves in the foot. At the least the current `std::vector<unsigned char>>` does not pretend to be secure.
💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903561636)
> I guess that by "plain text" here you mean `std::string`, right? `std::string` can store arbitrary non-ASCII characters, including `'\0'`, so it is technically ok to use it for binary data.
I think the way `WriteBinaryFile` was initially used with Tor was that we parse the JSON returned by the Tor daemon and store the `PrivateKey` key field (UTF8 encoced?) in a file.
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903561636)
> I guess that by "plain text" here you mean `std::string`, right? `std::string` can store arbitrary non-ASCII characters, including `'\0'`, so it is technically ok to use it for binary data.
I think the way `WriteBinaryFile` was initially used with Tor was that we parse the JSON returned by the Tor daemon and store the `PrivateKey` key field (UTF8 encoced?) in a file.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461554508)
> This seems underspecified: what's the behaviour if...
The behavior is what the existing RBF rules say (so no sibling eviction). If there's going to be 2 anchors I feel like v3 is not the right version to use.
> is it "we'll swap the old for the new if it has a better feerate diagram"?
I think sibling eviction can be considered, and feerate diagram comparisons as well. But the idea of splitting up #25038 was to decouple v3 topology restrictions from the (package) RBF rules that would b
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461554508)
> This seems underspecified: what's the behaviour if...
The behavior is what the existing RBF rules say (so no sibling eviction). If there's going to be 2 anchors I feel like v3 is not the right version to use.
> is it "we'll swap the old for the new if it has a better feerate diagram"?
I think sibling eviction can be considered, and feerate diagram comparisons as well. But the idea of splitting up #25038 was to decouple v3 topology restrictions from the (package) RBF rules that would b
...
💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903570460)
> I haven't looked in detail, but writing bytes to a file can be achieved with one line of code
That would be a nice simplification. Almost (?) to the point of not needing these helper functions.
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903570460)
> I haven't looked in detail, but writing bytes to a file can be achieved with one line of code
That would be a nice simplification. Almost (?) to the point of not needing these helper functions.
🚀 glozow merged a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260)
(https://github.com/bitcoin/bitcoin/pull/29260)
📝 jerzybrzoska opened a pull request: "Update build-unix.md - add boost library"
(https://github.com/bitcoin/bitcoin/pull/29290)
libboost-all-dev is needed to compile on Debian stable (Bookworm).
(https://github.com/bitcoin/bitcoin/pull/29290)
libboost-all-dev is needed to compile on Debian stable (Bookworm).
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1903697822)
> I find myself continually forgetting what the v3 rules are (particularly vs the ephemeral anchor rules, package relay rules, cluster mempool rules, or potential v4+ rules). It would be nice to have a really concise reference, that leaves the rationales/comparisons to an appendix or something. I think when stripped of historical baggage, it only needs to be:
> ...
I've consolidated the rules in the BIP (https://github.com/bitcoin/bips/pull/1541), agree it's easier to understand this way.
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1903697822)
> I find myself continually forgetting what the v3 rules are (particularly vs the ephemeral anchor rules, package relay rules, cluster mempool rules, or potential v4+ rules). It would be nice to have a really concise reference, that leaves the rationales/comparisons to an appendix or something. I think when stripped of historical baggage, it only needs to be:
> ...
I've consolidated the rules in the BIP (https://github.com/bitcoin/bips/pull/1541), agree it's easier to understand this way.
...
💬 dergoegge commented on pull request "Update build-unix.md - add boost library":
(https://github.com/bitcoin/bitcoin/pull/29290#issuecomment-1903703366)
The docs suggest to install or build boost one line below already: https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561bfd24b74f7271417defa5/doc/build-unix.md?plain=1#L49-L51
Does that not work for you?
(https://github.com/bitcoin/bitcoin/pull/29290#issuecomment-1903703366)
The docs suggest to install or build boost one line below already: https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561bfd24b74f7271417defa5/doc/build-unix.md?plain=1#L49-L51
Does that not work for you?
💬 ajtowns commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461597645)
It sounded good. 7 might be better: a tx that spends and creates an ephemeral anchor, as well as spending and creating a p2tr output (funding for fees plus change) should be about 164 vbytes, and 1000vb/164vb is about 6, for 7 total. Or you could just not limit the descendent count directly, but rely on the 1000vb limit to do it -- even with minimal 65vb txs, you'd only get 15 descendents in 1000vb.
NB: Just suggesting this as something that might be worth exploring, I don't think it should b
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461597645)
It sounded good. 7 might be better: a tx that spends and creates an ephemeral anchor, as well as spending and creating a p2tr output (funding for fees plus change) should be about 164 vbytes, and 1000vb/164vb is about 6, for 7 total. Or you could just not limit the descendent count directly, but rely on the 1000vb limit to do it -- even with minimal 65vb txs, you'd only get 15 descendents in 1000vb.
NB: Just suggesting this as something that might be worth exploring, I don't think it should b
...
💬 ajtowns commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461609384)
Maybe time to do:
```c++
const bool allow_rbf = ...;
if (allow_rbf) { ... }
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461609384)
Maybe time to do:
```c++
const bool allow_rbf = ...;
if (allow_rbf) { ... }
```
💬 ajtowns commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461602936)
Adding an additional child isn't an RBF event though -- the children don't "conflict" per se as they don't spend the same inputs -- so I don't think this is covered by RBF rules at all? It's fine if that's the behaviour, and fine if it's relaxed later; I just think it should just be documented explicitly here.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461602936)
Adding an additional child isn't an RBF event though -- the children don't "conflict" per se as they don't spend the same inputs -- so I don't think this is covered by RBF rules at all? It's fine if that's the behaviour, and fine if it's relaxed later; I just think it should just be documented explicitly here.
💬 ajtowns commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461544153)
Wouldn't it be better to distinguish these two reasons, ie introduce a `ZERODESCFEE` reason or so? Something like:
```c++
bool over_size = DynamicMemoryUsage() > sizelimit;
bool zerodescfee = (!over_size) && min_relay_fee > 0 && it->ModFeeWithDesc() <= 0;
if (!over_size && !zerodescfee) break;
...
RemoveStaged(stage, false, over_size ? SIZELIMIT : ZERODESCFEE);
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461544153)
Wouldn't it be better to distinguish these two reasons, ie introduce a `ZERODESCFEE` reason or so? Something like:
```c++
bool over_size = DynamicMemoryUsage() > sizelimit;
bool zerodescfee = (!over_size) && min_relay_fee > 0 && it->ModFeeWithDesc() <= 0;
if (!over_size && !zerodescfee) break;
...
RemoveStaged(stage, false, over_size ? SIZELIMIT : ZERODESCFEE);
```
💬 hebasto commented on pull request "refactor: Readable widget naming for debug window":
(https://github.com/bitcoin-core/gui/pull/790#issuecomment-1903714850)
@s1Sharp
Thank you for your contribution!
However, I'm going to close this PR following our [CONTRIBUTING Guide](https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#refactoring) and to avoid unnecessary conflicts with other PRs.
(https://github.com/bitcoin-core/gui/pull/790#issuecomment-1903714850)
@s1Sharp
Thank you for your contribution!
However, I'm going to close this PR following our [CONTRIBUTING Guide](https://github.com/bitcoin-core/gui/blob/master/CONTRIBUTING.md#refactoring) and to avoid unnecessary conflicts with other PRs.
✅ hebasto closed a pull request: "refactor: Readable widget naming for debug window"
(https://github.com/bitcoin-core/gui/pull/790)
(https://github.com/bitcoin-core/gui/pull/790)
💬 dergoegge commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1903721883)
Could this be the root cause for #27222?
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1903721883)
Could this be the root cause for #27222?
📝 theStack converted_to_draft a pull request: "libconsensus: adapt API header to be compliant to ANSI C"
(https://github.com/bitcoin/bitcoin/pull/28661)
libconsensus currently can't be used in projects that still use the [ANSI C](https://en.wikipedia.org/wiki/ANSI_C) (=ISO C/C89/C90) standard, as the header uses single-line-comments which are only supported since C99:
```
$ echo '#include "./src/script/bitcoinconsensus.h"\nint main() {}' > consensus_test.c
$ gcc -ansi -c consensus_test.c
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:1:1: error: C++ style comments are not allowed in ISO C90
1 | // Copyright
...
(https://github.com/bitcoin/bitcoin/pull/28661)
libconsensus currently can't be used in projects that still use the [ANSI C](https://en.wikipedia.org/wiki/ANSI_C) (=ISO C/C89/C90) standard, as the header uses single-line-comments which are only supported since C99:
```
$ echo '#include "./src/script/bitcoinconsensus.h"\nint main() {}' > consensus_test.c
$ gcc -ansi -c consensus_test.c
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:1:1: error: C++ style comments are not allowed in ISO C90
1 | // Copyright
...
💬 theStack commented on pull request "libconsensus: adapt API header to be compliant to ANSI C":
(https://github.com/bitcoin/bitcoin/pull/28661#issuecomment-1903733414)
> Could draft for now, dependant on the outcome of #29189?
Agree, done.
(https://github.com/bitcoin/bitcoin/pull/28661#issuecomment-1903733414)
> Could draft for now, dependant on the outcome of #29189?
Agree, done.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461685021)
Ok, I'll clarify that a second child is rejected regardless of fee(rate).
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461685021)
Ok, I'll clarify that a second child is rejected regardless of fee(rate).