💬 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 :)
💬 achow101 commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2161427580)
ACK 09ef322acc0a88a9e119f74923399598984c68f6
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2161427580)
ACK 09ef322acc0a88a9e119f74923399598984c68f6
💬 murchandamus commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161452991)
Sure! For context, the following Bitcoin Stack Exchange topic was what inspired me to open the issue: https://bitcoin.stackexchange.com/q/123331/5406
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2161452991)
Sure! For context, the following Bitcoin Stack Exchange topic was what inspired me to open the issue: https://bitcoin.stackexchange.com/q/123331/5406
🚀 achow101 merged a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830)
(https://github.com/bitcoin/bitcoin/pull/28830)
💬 Manuela123q commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1635384865)
Hhfft
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1635384865)
Hhfft
💬 achow101 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2161477616)
ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2161477616)
ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
🚀 achow101 merged a pull request: "cli: Detect port errors in rpcconnect and rpcport"
(https://github.com/bitcoin/bitcoin/pull/29521)
(https://github.com/bitcoin/bitcoin/pull/29521)
💬 achow101 commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-2161560686)
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-2161560686)
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1635456871)
> This means if the snapshot is not on the best chain, the node may be unable to sync to a better chain.
I've thought about this for a while, and I'm not sure if that problem is really being fixed by your suggestion for the following (slightly philosophical) reasons:
- Accepting a snapshot means that we've created a new chainstate with its Active Tip being set to that block.
- That is a commitment, we can't just reorg away from it to another chain that doesn't contain the snapshot block, ev
...
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1635456871)
> This means if the snapshot is not on the best chain, the node may be unable to sync to a better chain.
I've thought about this for a while, and I'm not sure if that problem is really being fixed by your suggestion for the following (slightly philosophical) reasons:
- Accepting a snapshot means that we've created a new chainstate with its Active Tip being set to that block.
- That is a commitment, we can't just reorg away from it to another chain that doesn't contain the snapshot block, ev
...
🚀 achow101 merged a pull request: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339)
(https://github.com/bitcoin/bitcoin/pull/28339)
💬 murchandamus commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-2161576108)
> > ```diff
> > --- a/test/functional/wallet_import_rescan.py
> > +++ b/test/functional/wallet_import_rescan.py
> > @@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
> > add_to_wallet=False,
> > inputs=[unspent_txid_map[variant.initial_txid]],
> > outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
> > + locktime=0,
> > subtract_fee_from_outputs=[0]
> > )
> >
...
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-2161576108)
> > ```diff
> > --- a/test/functional/wallet_import_rescan.py
> > +++ b/test/functional/wallet_import_rescan.py
> > @@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
> > add_to_wallet=False,
> > inputs=[unspent_txid_map[variant.initial_txid]],
> > outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
> > + locktime=0,
> > subtract_fee_from_outputs=[0]
> > )
> >
...
👍 brunoerg approved a pull request: "fuzz: add I2P harness"
(https://github.com/bitcoin/bitcoin/pull/30230#pullrequestreview-2111463661)
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
(https://github.com/bitcoin/bitcoin/pull/30230#pullrequestreview-2111463661)
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
💬 achow101 commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2161613165)
ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2161613165)
ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883