💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090946812)
i mean, there's probably some crappy C implementation of PCP out there that we could include, but this one is written in C++, it integrates with bitcoin's networking and logging, well commented, and it's straightforward enough. Besides, any code included from a third party would have to be reviewed just as well.
i've already agreed to add IPv4 support so it can replace libnatpmp.
@fanquake's comment was based on a misunderstanding.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090946812)
i mean, there's probably some crappy C implementation of PCP out there that we could include, but this one is written in C++, it integrates with bitcoin's networking and logging, well commented, and it's straightforward enough. Besides, any code included from a third party would have to be reviewed just as well.
i've already agreed to add IPv4 support so it can replace libnatpmp.
@fanquake's comment was based on a misunderstanding.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587919121)
pushed with the `Assume()` check inline with the "making sure package is cluster size two" check
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587919121)
pushed with the `Assume()` check inline with the "making sure package is cluster size two" check
🤔 hebasto reviewed a pull request: "Fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2036243242)
Approach ACK 0fea73fc7444884dc6831b1fe9e608898a398a92.
Linter failure?
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2036243242)
Approach ACK 0fea73fc7444884dc6831b1fe9e608898a398a92.
Linter failure?
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587940735)
i don't think that's the case. The mandatory flag is not a user choice, it's part of the RFC definition of the option:
- Figure 14 says "Option Code=2" only.
- Section 19.4 says "The values 0-127 are mandatory to process, and 128-255 are optional to process." This implies these are disjunct ranges.
In any case, we could be more lenient about this if we're willing to accept different addresses (or maybe even ports) and advertise them.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587940735)
i don't think that's the case. The mandatory flag is not a user choice, it's part of the RFC definition of the option:
- Figure 14 says "Option Code=2" only.
- Section 19.4 says "The values 0-127 are mandatory to process, and 128-255 are optional to process." This implies these are disjunct ranges.
In any case, we could be more lenient about this if we're willing to accept different addresses (or maybe even ports) and advertise them.
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587955428)
Right. Let's not do this. Such a stand-alone tool, if it is useful, doesn't need to be part of bitcoin.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587955428)
Right. Let's not do this. Such a stand-alone tool, if it is useful, doesn't need to be part of bitcoin.
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091032430)
In keeping with genesis block tradition I'd propose the following coinbase string referencing an article published today regarding testnet: "CCN 02/May/2024 Bitcoin Testnet Could Need Reset"
```
./generate-genesis -timestamp 1714658825 -pubkey 03752c999db32c08a6d62550a1ad83cf1731f35ac27096c27dfca5127c0c6dbd9b -psz "CCN 02/May/2024 Bitcoin Testnet Could Need Reset" -nonce 1
Ctrl Hash: 0x000000004693a387d558bbf7e321f6915e82d1afac4db25ee40f44bb783e802b
Target: 0x00000000ffff00000000000000000
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091032430)
In keeping with genesis block tradition I'd propose the following coinbase string referencing an article published today regarding testnet: "CCN 02/May/2024 Bitcoin Testnet Could Need Reset"
```
./generate-genesis -timestamp 1714658825 -pubkey 03752c999db32c08a6d62550a1ad83cf1731f35ac27096c27dfca5127c0c6dbd9b -psz "CCN 02/May/2024 Bitcoin Testnet Could Need Reset" -nonce 1
Ctrl Hash: 0x000000004693a387d558bbf7e321f6915e82d1afac4db25ee40f44bb783e802b
Target: 0x00000000ffff00000000000000000
...
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091039789)
> Only allowing lower difficulty blocks after 6 hours could easily make testnet useless for extended periods, if someone put several ASICs on testnet for a while, it might prevent other users from getting confirmations for up to 6 hours. I could see an increase from the twenty minute rule to maybe an hour, but more seems counter to why the rule was introduced in the first place.
6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be a
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091039789)
> Only allowing lower difficulty blocks after 6 hours could easily make testnet useless for extended periods, if someone put several ASICs on testnet for a while, it might prevent other users from getting confirmations for up to 6 hours. I could see an increase from the twenty minute rule to maybe an hour, but more seems counter to why the rule was introduced in the first place.
6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be a
...
💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1587963767)
Could also continue to get all the UTXOs up front?
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1587963767)
Could also continue to get all the UTXOs up front?
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587973156)
I was looking at 7.3:
> Option Code: 8 bits. Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
But that's implied by using these integer ranges.
Tend to agree we should drop this option, though we should pay attention to its return value. Otherwise we'd potentially gossip the wrong port (in practice this likely only happens if you have two nodes in the same home).
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587973156)
I was looking at 7.3:
> Option Code: 8 bits. Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
But that's implied by using these integer ranges.
Tend to agree we should drop this option, though we should pay attention to its return value. Otherwise we'd potentially gossip the wrong port (in practice this likely only happens if you have two nodes in the same home).
📝 jonatack opened a pull request: "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE"
(https://github.com/bitcoin/bitcoin/pull/30024)
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
(https://github.com/bitcoin/bitcoin/pull/30024)
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
💬 instagibbs commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091169327)
maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else? concept ACK otherwise. I can confirm they're all referring to the same limit at least.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091169327)
maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else? concept ACK otherwise. I can confirm they're all referring to the same limit at least.
💬 sdaftuar commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1588067475)
Inspired by your approach, I thought it might be helpful to just add support for setting a target feerate on a transaction in miniwallet, something like this?
```diff
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..db6aa78167 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -533,13 +533,25 @@ class MempoolAcceptV3(BitcoinTestFramework):
tx_unrelated_replacee = self.wallet.s
...
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1588067475)
Inspired by your approach, I thought it might be helpful to just add support for setting a target feerate on a transaction in miniwallet, something like this?
```diff
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..db6aa78167 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -533,13 +533,25 @@ class MempoolAcceptV3(BitcoinTestFramework):
tx_unrelated_replacee = self.wallet.s
...
💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091226696)
> 6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be able to generate a block 1 hour after the previous block if they also warped the time ahead by the max 2 hours.
Three hours seems too long to me. That would mean that the first use of the low-difficulty loophole would require one hour of wait (by also setting your clock two hours to the future), but any subsequent block would be delayed by three hours. It would be trivial for an
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091226696)
> 6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be able to generate a block 1 hour after the previous block if they also warped the time ahead by the max 2 hours.
Three hours seems too long to me. That would mean that the first use of the low-difficulty loophole would require one hour of wait (by also setting your clock two hours to the future), but any subsequent block would be delayed by three hours. It would be trivial for an
...
💬 sipsorcery commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1588166620)
This check will fail on Windows. I'm guessing that's expected but just checking?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1588166620)
This check will fail on Windows. I'm guessing that's expected but just checking?
💬 vasild commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091280337)
This is indeed some problem in FreeBSD's libc which I reported upstream with a minimal, reproducable test case:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701
Is anybody interested in reviewing the patch I posted above if I PR it:
https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241
reworked to return `std::string_view` to the callers, but still storing the thread name in `char []`? That would work around the FreeBSD weird message printout because the `thread
...
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091280337)
This is indeed some problem in FreeBSD's libc which I reported upstream with a minimal, reproducable test case:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701
Is anybody interested in reviewing the patch I posted above if I PR it:
https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241
reworked to return `std::string_view` to the callers, but still storing the thread name in `char []`? That would work around the FreeBSD weird message printout because the `thread
...
💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#issuecomment-2091285732)
ping @maflcko
(https://github.com/bitcoin/bitcoin/pull/29421#issuecomment-2091285732)
ping @maflcko
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1588204550)
The goal is to detect that binaries, which are cross-compiled for Windows, are running in Wine.
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1588204550)
The goal is to detect that binaries, which are cross-compiled for Windows, are running in Wine.
💬 laanwj commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091354481)
> reworked to return std::string_view to the callers, but still storing the thread name in a thread_local char []?
Yes. i think (for this one very rare exception) it's acceptable to store a string in a fixed-size buffer. To not need a destructor and heap deallocation when a thread goes away, works around a large part of the complexity of handling thread-local data.
And making it use `string_view` throughout the changed functions instead of `char*` is a good idea, a lot less ugly.
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091354481)
> reworked to return std::string_view to the callers, but still storing the thread name in a thread_local char []?
Yes. i think (for this one very rare exception) it's acceptable to store a string in a fixed-size buffer. To not need a destructor and heap deallocation when a thread goes away, works around a large part of the complexity of handling thread-local data.
And making it use `string_view` throughout the changed functions instead of `char*` is a good idea, a lot less ugly.
💬 jonatack commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091356712)
> maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else?
Thanks @instagibbs, done.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091356712)
> maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else?
Thanks @instagibbs, done.
💬 laanwj commented on issue "importaddress failed, Only legacy wallets are supported by this command.":
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2091363954)
Yes, adding an example would be useful.
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2091363954)
Yes, adding an example would be useful.