💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2671912617)
I'm sad too! Seeing the stats from https://delvingbitcoin.org/t/stats-on-orphanage-overflows/1421 made this more pressing in my opinion, but it's not a regression. I think we can still try to consider small, obviously safe changes for v29, but this feels too big. I don't want to risk creating new DoS problems.
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2671912617)
I'm sad too! Seeing the stats from https://delvingbitcoin.org/t/stats-on-orphanage-overflows/1421 made this more pressing in my opinion, but it's not a regression. I think we can still try to consider small, obviously safe changes for v29, but this feels too big. I don't want to risk creating new DoS problems.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963845813)
I think I am starting to grasp: `fallback_os_search` (or previously `search_system_path`) is about allowing this code:
```cpp
181 // Decide whether to fall back to the operating system to search for the
182 // specified executable. Avoid doing this if it looks like the wrapper
183 // executable was invoked by path, rather than by search, to avoid
184 // unintentionally launching system executables in a local build.
185 // (https://github.com/bitcoin/bitcoin/pull/3137
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963845813)
I think I am starting to grasp: `fallback_os_search` (or previously `search_system_path`) is about allowing this code:
```cpp
181 // Decide whether to fall back to the operating system to search for the
182 // specified executable. Avoid doing this if it looks like the wrapper
183 // executable was invoked by path, rather than by search, to avoid
184 // unintentionally launching system executables in a local build.
185 // (https://github.com/bitcoin/bitcoin/pull/3137
...
💬 maflcko commented on pull request "doc: navigate section links":
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1963854603)
Reference that it worked in the past: https://web.archive.org/web/20241112104825/https://clang.llvm.org/docs/AttributeReference.html#lifetimebound
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1963854603)
Reference that it worked in the past: https://web.archive.org/web/20241112104825/https://clang.llvm.org/docs/AttributeReference.html#lifetimebound
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963862052)
I don't have any particular suggestion. I will post here if I come up with some significant simplifications.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1963862052)
I don't have any particular suggestion. I will post here if I come up with some significant simplifications.
📝 laanwj opened a pull request: "init: Handle dropped UPnP support more gracefully"
(https://github.com/bitcoin/bitcoin/pull/31916)
Closes bitcoin-core/gui#843.
In that issue it was brought up that users likely don't care what kind of port forwarding is used, and that the setting is opportunistic anyway, so instead of showing an extensive warning, we can simply "upgrade" from UPNP to NAT-PMP+PCP.
- Change the logic for removed runtime setting `-upnp` to set `-natpmp` instead, and log a message.
- Also remove any lingering `upnp` from `settings.json` and replace it with `natpmp`, when it makes sense (for the UI):
...
(https://github.com/bitcoin/bitcoin/pull/31916)
Closes bitcoin-core/gui#843.
In that issue it was brought up that users likely don't care what kind of port forwarding is used, and that the setting is opportunistic anyway, so instead of showing an extensive warning, we can simply "upgrade" from UPNP to NAT-PMP+PCP.
- Change the logic for removed runtime setting `-upnp` to set `-natpmp` instead, and log a message.
- Also remove any lingering `upnp` from `settings.json` and replace it with `natpmp`, when it makes sense (for the UI):
...
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)
Removed the `else`.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)
Removed the `else`.
👍 theStack approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2630429098)
Lightly tested re-ACK f7d8a44ce42cce0b3010a7f7d14ec180e959f3e4
(retested as described in https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2578563294 before)
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2630429098)
Lightly tested re-ACK f7d8a44ce42cce0b3010a7f7d14ec180e959f3e4
(retested as described in https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2578563294 before)
💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2671982208)
Can confirm behavior described by @Sjors. Seems like apple is confusing itself?
<img src="https://github.com/user-attachments/assets/1774cca4-92e1-4c08-a8cc-34fe195c2cc0" width="300">
but binary is signed:
```
--> codesign -vd --verbose=4 /Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Executable=/Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Identifier=bitcoind
Format=Mach-O thin (arm64)
CodeDirectory v=20500 size=23284 flags=0x10000(runtime) hashes
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2671982208)
Can confirm behavior described by @Sjors. Seems like apple is confusing itself?
<img src="https://github.com/user-attachments/assets/1774cca4-92e1-4c08-a8cc-34fe195c2cc0" width="300">
but binary is signed:
```
--> codesign -vd --verbose=4 /Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Executable=/Users/matthewzipkin/Desktop/bitcoin-e181bda061ca/bin/bitcoind
Identifier=bitcoind
Format=Mach-O thin (arm64)
CodeDirectory v=20500 size=23284 flags=0x10000(runtime) hashes
...
💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963919027)
Is this the right check? IIUC `IsArgSet` will return true even when the value is negated (`-noupnp`)?
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963919027)
Is this the right check? IIUC `IsArgSet` will return true even when the value is negated (`-noupnp`)?
👍 theuni approved a pull request: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884#pullrequestreview-2630455667)
utACK 3b42e05aa9e38ba00d52b2338375b4caf032f041
Thanks for the fix!
(https://github.com/bitcoin/bitcoin/pull/31884#pullrequestreview-2630455667)
utACK 3b42e05aa9e38ba00d52b2338375b4caf032f041
Thanks for the fix!
💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1963931431)
I do agree to add `never` to the list of timestamp values to handle this case.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1963931431)
I do agree to add `never` to the list of timestamp values to handle this case.
📝 l0rinc opened a pull request: "fuzz: provide more realistic values to the base58(check) decoders"
(https://github.com/bitcoin/bitcoin/pull/31917)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* providing more valid values to the decoder (suggested by @maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one;
* capping max sizes to `100` instead of `1000` (suggested by @marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `
...
(https://github.com/bitcoin/bitcoin/pull/31917)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* providing more valid values to the decoder (suggested by @maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one;
* capping max sizes to `100` instead of `1000` (suggested by @marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `
...
💬 l0rinc commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963935857)
Pushed https://github.com/bitcoin/bitcoin/pull/31917, thanks @maflcko & @marcofleon.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963935857)
Pushed https://github.com/bitcoin/bitcoin/pull/31917, thanks @maflcko & @marcofleon.
👍 pinheadmz approved a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2630480382)
ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
Reviewed changes since last ACK, re-reviewed all changes, tested locally by creating a `TestShell` and sending RPC commands
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAme3V0wACgkQ5+KYS2KJ
yTqd1xAA1DppfrHcaHcdKp/HnsicUqMslnuQ6sKFoQK76EYEsQFTyjJi2QDno8iB
g4LI7wpfse3
...
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2630480382)
ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
Reviewed changes since last ACK, re-reviewed all changes, tested locally by creating a `TestShell` and sending RPC commands
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAme3V0wACgkQ5+KYS2KJ
yTqd1xAA1DppfrHcaHcdKp/HnsicUqMslnuQ6sKFoQK76EYEsQFTyjJi2QDno8iB
g4LI7wpfse3
...
💬 marcofleon commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1963946848)
The problem actually turned out to be this. The input that times out is about 1MB so passing in the whole buffer causes the timeout.
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1963946848)
The problem actually turned out to be this. The input that times out is about 1MB so passing in the whole buffer causes the timeout.
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963948947)
Good catch, this is the wrong check. For the settings.json case i did consider the "is set" and "is set to true" case differently but consider them the same here.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963948947)
Good catch, this is the wrong check. For the settings.json case i did consider the "is set" and "is set to true" case differently but consider them the same here.
💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963961846)
I haven't tried to compile, but the modern way of my suggestion would be:
```cpp
if (const auto arg{args.GetBoolArg("-upnp")}) {
bool overridden = args.SoftSetBoolArg("-natpmp", *arg);
```
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1963961846)
I haven't tried to compile, but the modern way of my suggestion would be:
```cpp
if (const auto arg{args.GetBoolArg("-upnp")}) {
bool overridden = args.SoftSetBoolArg("-natpmp", *arg);
```
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963132)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:
```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963132)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:
```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963925)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:
```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963963925)
I guess my worry is that we would somehow add a regression that results in the peer set growing indefinitely in the orphanage, which would grow memory usage substantially. Here's some suggested coverage which would hopefully uncover such an issue:
```
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9133323449..df0a02d75d 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -45,10 +45,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanag
...
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963975702)
You are right. That explains why it doesn't get slower haha
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1963975702)
You are right. That explains why it doesn't get slower haha