💬 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).
💬 ajtowns commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903768404)
> I'm somewhat puzzled why Knots (cc @luke-jr) and Inquisition (cc @ajtowns) are not running into the same issues. But I guess there's at least two differences:
Inquisition disabled all the automatic tests except the trivial linter when cirrus imposed limits earlier in the year, and tests are only run manually by reviewers. Haven't worked out what it should be doing, so don't have suggestions to offer, but interested to see what the resolution here is.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903768404)
> I'm somewhat puzzled why Knots (cc @luke-jr) and Inquisition (cc @ajtowns) are not running into the same issues. But I guess there's at least two differences:
Inquisition disabled all the automatic tests except the trivial linter when cirrus imposed limits earlier in the year, and tests are only run manually by reviewers. Haven't worked out what it should be doing, so don't have suggestions to offer, but interested to see what the resolution here is.
💬 willcl-ark commented on issue "berkeley database failed to open database environment":
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1903822925)
Hello @freezoloto. Can you confirm that there is a wallet database directory at that path? This error is primarily thrown when that directory does not exist, but can be thrown for other reasons, so it would be good to confirm that it does exist before trying to investigate further.
On Linux (or WSL) this could be done by running something like this:
```bash
will in ~ :
$ cd .bitcoin
will in ~/.bitcoin:
$ ls -al
.rw------- 75 will 22 Jan 09:15 .cookie
.rw------- 0 will 12 Jan 1
...
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1903822925)
Hello @freezoloto. Can you confirm that there is a wallet database directory at that path? This error is primarily thrown when that directory does not exist, but can be thrown for other reasons, so it would be good to confirm that it does exist before trying to investigate further.
On Linux (or WSL) this could be done by running something like this:
```bash
will in ~ :
$ cd .bitcoin
will in ~/.bitcoin:
$ ls -al
.rw------- 75 will 22 Jan 09:15 .cookie
.rw------- 0 will 12 Jan 1
...
💬 moemat12 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1461745349)
src/wallet/sqlite.h
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1461745349)
src/wallet/sqlite.h
💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1461750521)
> CConMan cannot access the chain state, the sync progress, in-flight block requests
I don't see how the connection opening logic needs access to any of those things.
> number of compact block filters
Bitcoin Core doesn't download compact block filters.
> v2 encrypted connections, etc.
Connman is doing this right now using our service flags and the service flag of the potential new peer. It has all the data it needs.
> Essentially, in my view, the approach is resource-wasteful
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1461750521)
> CConMan cannot access the chain state, the sync progress, in-flight block requests
I don't see how the connection opening logic needs access to any of those things.
> number of compact block filters
Bitcoin Core doesn't download compact block filters.
> v2 encrypted connections, etc.
Connman is doing this right now using our service flags and the service flag of the potential new peer. It has all the data it needs.
> Essentially, in my view, the approach is resource-wasteful
...
✅ jerzybrzoska closed a pull request: "Update build-unix.md - add boost library"
(https://github.com/bitcoin/bitcoin/pull/29290)
(https://github.com/bitcoin/bitcoin/pull/29290)
💬 jerzybrzoska commented on pull request "Update build-unix.md - add boost library":
(https://github.com/bitcoin/bitcoin/pull/29290#issuecomment-1903861175)
> 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?
Yes it does.
(https://github.com/bitcoin/bitcoin/pull/29290#issuecomment-1903861175)
> 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?
Yes it does.
👍 hebasto approved a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1836318583)
ACK d664eab66ecfa1d318b11de5deafb04b1e3c4c06, tested on Ubuntu 23.10.
nano-nit: There are some complains from [`clang-format-diff.py`](https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy).
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1836318583)
ACK d664eab66ecfa1d318b11de5deafb04b1e3c4c06, tested on Ubuntu 23.10.
nano-nit: There are some complains from [`clang-format-diff.py`](https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy).
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461755517)
Specify the restrictions is only for unconfirmed v3 transaction here and other places.
```suggestion
// v3 only allows 1 unconfirmed parent and 1 unconfirmed child.
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461755517)
Specify the restrictions is only for unconfirmed v3 transaction here and other places.
```suggestion
// v3 only allows 1 unconfirmed parent and 1 unconfirmed child.
```
🤔 ismaelsadeeq reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1836296374)
> I've consolidated the rules in the BIP ([bitcoin/bips#1541](https://github.com/bitcoin/bips/pull/1541)), agree it's easier to understand this way.
I've looked at the BIP and this implementation matches the specification in the BIP
> I'll delete or clean up the version3_transactions.md doc here now that the information has been moved to the BIP.
+1
Changes since my [last review](https://github.com/bitcoin/bitcoin/compare/1dd62c3df4856c36bfc610f700684852772dd9f7..f3d4916eacfbcef61b
...
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1836296374)
> I've consolidated the rules in the BIP ([bitcoin/bips#1541](https://github.com/bitcoin/bips/pull/1541)), agree it's easier to understand this way.
I've looked at the BIP and this implementation matches the specification in the BIP
> I'll delete or clean up the version3_transactions.md doc here now that the information has been moved to the BIP.
+1
Changes since my [last review](https://github.com/bitcoin/bitcoin/compare/1dd62c3df4856c36bfc610f700684852772dd9f7..f3d4916eacfbcef61b
...
💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903992967)
> > 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.
WriteBinaryFile is unused right now either way outside of tests, so I guess this could be removed regardless, as future code can just use the in-line one-liner?
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903992967)
> > 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.
WriteBinaryFile is unused right now either way outside of tests, so I guess this could be removed regardless, as future code can just use the in-line one-liner?