💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635107884)
Nice, agree that's better. However e4e14fedc9622ca7cfc40af4f2aa70ed4bb7daa6 is now unnecessary and can be dropped.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635107884)
Nice, agree that's better. However e4e14fedc9622ca7cfc40af4f2aa70ed4bb7daa6 is now unnecessary and can be dropped.
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635108539)
I think so...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635108539)
I think so...
🚀 fanquake merged a pull request: "depends: remove `FORCE_USE_SYSTEM_CLANG`"
(https://github.com/bitcoin/bitcoin/pull/30201)
(https://github.com/bitcoin/bitcoin/pull/30201)
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635112115)
Aha yes. Can resolve.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635112115)
Aha yes. Can resolve.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635117157)
even better. dropped.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1635117157)
even better. dropped.
💬 theuni commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2161096437)
Does ` __attribute__ ((visibility ("default")))` override this? Otherwise I assume we'd need to make sure this flag doesn't make it to shared libs.
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2161096437)
Does ` __attribute__ ((visibility ("default")))` override this? Otherwise I assume we'd need to make sure this flag doesn't make it to shared libs.
💬 theuni commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2161099989)
> Does ` __attribute__ ((visibility ("default")))` override this? Otherwise I assume we'd need to make sure this flag doesn't make it to shared libs.
Oh whoops, already asked and answered. Nevermind!
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2161099989)
> Does ` __attribute__ ((visibility ("default")))` override this? Otherwise I assume we'd need to make sure this flag doesn't make it to shared libs.
Oh whoops, already asked and answered. Nevermind!
👍 tdb3 approved a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2110894428)
ACK cbc604f6a7739bf22a5d811b6366453860d59f4c
Seems like an improvement to avoid expending resources loading a snapshot that won't be part of a valid chain.
Built and ran unit and functional tests (including `feature_assumeutxo`). All passed.
Tested that `feature_assumeutxo` would fail with:
```c++
bool start_block_invalid = false;
```
Test failed as expected.
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2110894428)
ACK cbc604f6a7739bf22a5d811b6366453860d59f4c
Seems like an improvement to avoid expending resources loading a snapshot that won't be part of a valid chain.
Built and ran unit and functional tests (including `feature_assumeutxo`). All passed.
Tested that `feature_assumeutxo` would fail with:
```c++
bool start_block_invalid = false;
```
Test failed as expected.
💬 tdb3 commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1635144969)
> If the base block of the snapshot is marked invalid or part of an invalid chain
This existing test seems to focus on the base block being part of an invalid chain. Is it also worth adding an explicit test for an invalid base block itself (or would it be overkill)?
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1635144969)
> If the base block of the snapshot is marked invalid or part of an invalid chain
This existing test seems to focus on the base block being part of an invalid chain. Is it also worth adding an explicit test for an invalid base block itself (or would it be overkill)?
💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1635147026)
> Particularly "but has less work" could mean A) less work than the tip of the chain that includes the snapshot or B) less work than the snapshot block itself. I believe A is the more interesting and intended scenario because B only seems to be a state a node can be in if the snapshot block is invalid or its ancestors are unknown which should lead to a much earlier error.
I interpreted it as B) less work than the snapshot block itself.
Regarding A): If the new node (divergent chain) has le
...
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1635147026)
> Particularly "but has less work" could mean A) less work than the tip of the chain that includes the snapshot or B) less work than the snapshot block itself. I believe A is the more interesting and intended scenario because B only seems to be a state a node can be in if the snapshot block is invalid or its ancestors are unknown which should lead to a much earlier error.
I interpreted it as B) less work than the snapshot block itself.
Regarding A): If the new node (divergent chain) has le
...
📝 theuni opened a pull request: "utils: add missing VecDeque include"
(https://github.com/bitcoin/bitcoin/pull/30268)
Noticed when testing `VecDeque` with no other includes.
For libc++, need type_traits for `std::is_trivially_destructible_v`.
(https://github.com/bitcoin/bitcoin/pull/30268)
Noticed when testing `VecDeque` with no other includes.
For libc++, need type_traits for `std::is_trivially_destructible_v`.
👍 theuni approved a pull request: "refactor: reserve memory allocation for transaction outputs"
(https://github.com/bitcoin/bitcoin/pull/30093#pullrequestreview-2110966158)
utACK db2a31c339ad697f1828cde162f4df2231c9b0f1
(https://github.com/bitcoin/bitcoin/pull/30093#pullrequestreview-2110966158)
utACK db2a31c339ad697f1828cde162f4df2231c9b0f1
💬 ryanofsky commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2161208967)
Hmm, this is the first time I've seen the -Wundef warning, and my initial reaction is to be skeptical of it, because it seems like it would require patching a lot of upstream headers, and I'm not sure if it it just forces writing `#if defined(SYMBOL)` instead of `#if SYMBOL` everywhere it is likely to prevent any bugs. It seems more like it is breaking a commonly used idiom.
But if I'm not seeing the benefits, or others just think Wundef is more useful, I can create a patch for depends and an
...
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2161208967)
Hmm, this is the first time I've seen the -Wundef warning, and my initial reaction is to be skeptical of it, because it seems like it would require patching a lot of upstream headers, and I'm not sure if it it just forces writing `#if defined(SYMBOL)` instead of `#if SYMBOL` everywhere it is likely to prevent any bugs. It seems more like it is breaking a commonly used idiom.
But if I'm not seeing the benefits, or others just think Wundef is more useful, I can create a patch for depends and an
...
💬 theStack commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#issuecomment-2161224039)
Rebased on master, after the merge of #30162. Adding a functional test for tagged wallets to the new `feature_framework_miniwallet.py` (see commit e4b0dabb2115dc74e9c5794ddca3822cd8301c72) revealed another bug w.r.t. the internal key derivation. The code on master wrongly assumes that every pseudorandom hash results in a valid x-only pubkey. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key (see commit 3162c917e93
...
(https://github.com/bitcoin/bitcoin/pull/30076#issuecomment-2161224039)
Rebased on master, after the merge of #30162. Adding a functional test for tagged wallets to the new `feature_framework_miniwallet.py` (see commit e4b0dabb2115dc74e9c5794ddca3822cd8301c72) revealed another bug w.r.t. the internal key derivation. The code on master wrongly assumes that every pseudorandom hash results in a valid x-only pubkey. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key (see commit 3162c917e93
...
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1635224220)
The purpose is for `PreRegisterPeer` to be callable with a fixed salt (via `PreRegisterPeerWithSalt`), so collisions can be tested. This is just moving it out of the `PImpl`, the effects on the external caller should be the same
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1635224220)
The purpose is for `PreRegisterPeer` to be callable with a fixed salt (via `PreRegisterPeerWithSalt`), so collisions can be tested. This is just moving it out of the `PImpl`, the effects on the external caller should be the same
⚠️ murchandamus opened an issue: "Improve description of the `filename` parameter of `loadwallet` RPC"
(https://github.com/bitcoin/bitcoin/issues/30269)
### Motivation
As the documentation for the [`loadwallet`](https://bitcoincore.org/en/doc/27.0.0/rpc/wallet/loadwallet/) RPC describes, the syntax is:
loadwallet "filename" ( load_on_startup )
And it further specifies:
Arguments:
1. filename (string, required) The wallet directory or .dat file.
…
What’s not clearly specified in the documentation is that you are to provide the filename _relative to the wallet directory_ `~/.bitcoin/regtest/wallets`. It would be grea
...
(https://github.com/bitcoin/bitcoin/issues/30269)
### Motivation
As the documentation for the [`loadwallet`](https://bitcoincore.org/en/doc/27.0.0/rpc/wallet/loadwallet/) RPC describes, the syntax is:
loadwallet "filename" ( load_on_startup )
And it further specifies:
Arguments:
1. filename (string, required) The wallet directory or .dat file.
…
What’s not clearly specified in the documentation is that you are to provide the filename _relative to the wallet directory_ `~/.bitcoin/regtest/wallets`. It would be grea
...
💬 marcofleon commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635236868)
Yes, `GetTime()` is also called in `SendComplete`.
> Does this mean that the time will be frozen for the duration of the test?
I believe so, unless I'm missing something. For each iteration, `g_mock_time` will be set to an integer within the range specified in `ConsumeTime`.
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635236868)
Yes, `GetTime()` is also called in `SendComplete`.
> Does this mean that the time will be frozen for the duration of the test?
I believe so, unless I'm missing something. For each iteration, `g_mock_time` will be set to an integer within the range specified in `ConsumeTime`.
💬 marcofleon commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635237667)
This doesn't seem to work when I test it. I think this results in an unspecifed IPv6 address, so in `Hello()`, the sock returned from `m_control_host.Connect()` is always invalid.
https://github.com/bitcoin/bitcoin/blob/337f9d44c28b1d3563a0063a8805b418d506698d/src/netaddress.cpp#L425-L431
A possible option is to use `ConsumeService` here instead of explicitly setting the `addr`. I haven't tested it yet but would probably work.
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635237667)
This doesn't seem to work when I test it. I think this results in an unspecifed IPv6 address, so in `Hello()`, the sock returned from `m_control_host.Connect()` is always invalid.
https://github.com/bitcoin/bitcoin/blob/337f9d44c28b1d3563a0063a8805b418d506698d/src/netaddress.cpp#L425-L431
A possible option is to use `ConsumeService` here instead of explicitly setting the `addr`. I haven't tested it yet but would probably work.
💬 MukulKolpe commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161272201)
Hey @murchandamus, can I work on this issue?
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161272201)
Hey @murchandamus, can I work on this issue?
💬 willcl-ark commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161360992)
@MukulKolpe absolutely. We don't generally assign issues in this project. Anyone is free to open a PR fixing any issue.
As @murchandamus advised in OP, please take care to:
> ... read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md?rgh-link-date=2024-06-11T17%3A09%3A29Z) before opening your pull request.
If you have any questions about the issue at hand here, or generally opening a Pull Request in this repo, feel free to ask away in here :)
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161360992)
@MukulKolpe absolutely. We don't generally assign issues in this project. Anyone is free to open a PR fixing any issue.
As @murchandamus advised in OP, please take care to:
> ... read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md?rgh-link-date=2024-06-11T17%3A09%3A29Z) before opening your pull request.
If you have any questions about the issue at hand here, or generally opening a Pull Request in this repo, feel free to ask away in here :)